Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hero alt text #889

Merged
merged 3 commits into from
Nov 5, 2024
Merged

Fix hero alt text #889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
788f7e5
Grep for errors when invoking Lambda management function
symroe Nov 5, 2024
a5ac2b5
Fix typo in awsgi file name
symroe Nov 5, 2024
faadb5a
Drive by performance improvements
symroe Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,11 @@ jobs:
- aws-cli/setup
- run:
name: "Migrate database"
# Invoke Lambda using the AWS CLI. Grep for errors in the response because
# the command will exit 0 even if the invocation failed. The JSON key `FunctionError` will contain something
# if there's an error.
command: |
aws lambda invoke --function-name DCWebsiteManagementFunction --payload '{ "command": "migrate", "args": ["--no-input"] }' --cli-binary-format raw-in-base64-out -
aws lambda invoke --function-name DCWebsiteManagementFunction --payload '{ "command": "migrate", "args": ["--no-input"] }' --cli-binary-format raw-in-base64-out - | grep 'FunctionError": ""'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while to work out what we're trying to do here is exit with a non-zero if FunctionError is equal to some value other than empty string "". Might be useful to add a comment to this one.

Some follow up questions:

  1. This will fail the build if FunctionError is equal to something other than empty string, which is good. But as far as I can see there's nothing in this PR that actually aims to fix the migrations failing. Do we know why this is happening/what the fix is? i.e: this change in itself won't cause the migration to apply correctly next time we deploy.
  2. At the point where we attempt to apply the migrations, we've already deployed the new release. So if we fail the build here, we deploy the new lambda, but then we'll just stop here (and not invalidate the CF cache or make a new sentry release). Is that right, and if so, is that a good outcome? i.e: is deploying new code and not invalidating the CloudFront cache potentially creating another class of problem?
  3. Is this a problem we think exists on other DC projects?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh.. hang on.

Is

-      Handler: democracy_club.lambda_awsgi.management_handler
+      Handler: democracy_club.lambda_wsgi.management_handler

the fix for failing migrations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sorry, I didn't write a lot of explanation here!

The problem is the typo above: we invoke a Lambda that exists but it throws a 500 because the handler code doesn't exist at that path.

The invocation "works" in that, we have permission to call a function that exists, but "fails" because of this error.

This means we need a way to get a return code that's non-0 somehow, and I thought grepping the JSON was...a way of doing that. I can't see anything in the aws-cli docs about exit codes in this situation.

In terms of when we run the code and the implication on the deploy...I don't know. We should run the migrations after the code has changed, but how to rollback a failed deploy is a bigger question. At least this makes the error a hard fail that we can be notified about, rather than just working without us knowing.

In terms of other projects...I don't think so. Maybe WCIVF does the same sort of thing?

Copy link
Member

@chris48s chris48s Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yeah OK. We basically have no clean rollback for failed deploys, so that's a bigger issue.
  1. I think in lieu of any better way to assess success/failure of this command, grep is fine but lets explain in a comment what we're doing here.
  1. In terms of other repos affected by this issue, I think the EC2/CDK-based repos are probably fine. YNR is its own special beast.

It looks like if we have this problem here then I reckon we have the same issue on:

WCIVF
https://github.com/DemocracyClub/WhoCanIVoteFor/blob/36d9c34d6a04eaea2cc9bf9f81815354a6bfd27d/Makefile#L26-L31

devs.DC
https://github.com/DemocracyClub/aggregator-api/blob/0d9c876c619979392211d41a4c346a3a3213b80c/.circleci/config.yml#L137-L140

so lets fix those as well, rather than just leaving a known issue as a trap for ourselves to fall into in future.

Then the other 2 places I expected to find this issue were leaflets and ec-api-proxy. They both appear to be running the migrations against the prod database directly from CI instead of invoking a lambda

which is.. different
Is there a reason we've taken different approaches to this in different places? (I realise WCIVF is an outlier compared to the other apps we're looking at here as it is EC2/SAM rather than lambda/SAM)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, pushed a comment

- run:
name: "Invalidate CloudFront cache"
command: |
Expand Down
9 changes: 6 additions & 3 deletions democracy_club/apps/hermes/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,20 @@ class PostListView(ListView):
template_name = "hermes/post_list.html"

def get_queryset(self):
qs = self.model.objects.published().prefetch_related("author")
qs = (
self.model.objects.published()
.prefetch_related("author")
.defer("body", "hero_alt_text")
)
tag = self.request.GET.get("tag", None)
if tag:
qs = qs.for_tag(tag)
return qs

def get_context_data(self, **kwargs):
context = super(PostListView, self).get_context_data(**kwargs)
posts = Post.objects.all().published()
tags = []
for post in posts:
for post in context["object_list"]:
for tag in post.tags:
if tag not in tags:
tags.append(tag)
Expand Down
2 changes: 1 addition & 1 deletion sam-template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ Resources:
Timeout: 60
Role: !Sub "arn:aws:iam::${AWS::AccountId}:role/DCWebsiteLambdaExecutionRole"
CodeUri: .
Handler: democracy_club.lambda_awsgi.management_handler
Handler: democracy_club.lambda_wsgi.management_handler
Layers:
- !Ref DependenciesLayer
Runtime: python3.12
Expand Down