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.

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

Compressor redux #794

Merged
merged 11 commits into from Jan 13, 2021
Merged

Compressor redux #794

merged 11 commits into from Jan 13, 2021

Conversation

cantsin
Copy link
Contributor

@cantsin cantsin commented Jan 5, 2021

Link to ZenHub issue.

What does this change?

Second try at implementing django-compressor functionality. Believe it or not, the hero.png image (which we never use) is indeed needed and breaks manifest storage outright if we omit this image. Why? Well, the USWDS CSS specifies background-image:url("../img/hero.png"); which the Django manifest storage backend picks up and then subsequently crashes during collectstatic when this file is not found. tl;dr: I believe this missing, unused file is what broke deployment last time (really).

The rest is identical to before, except for a couple of small things. We can use the built in S3ManifestStaticStorage instead of monkey patching classes. Two CSS url references have been tweaked so that they are properly fetched from S3.

But most exciting is the localstack documentation I've thrown in, which explains how to set up an identical-ish production environment locally.

Screenshots (for front-end PR):

2021-01-05_15-39

Checklist:

Author

  • If this is a story, run locally and check to make sure all Acceptance Criteria are met. (If any criteria are unclear, ask about them.).
  • Check for, document, and establish a testing plan for any behavior that may vary across environments or is otherwise difficult to test.
  • Check for accessibility.
  • Tests pass.

Reviewer

  • If this is a story, run locally and check to make sure all Acceptance Criteria are met. (If any criteria are unclear, ask about them.).
  • Check for any behavior that may vary across environments or is difficult to test, and ensure that it is well-understood, documented, and that there is a testing plan in place.
  • Re-check for accessibility.
  • Tests pass.

Notes for reviewer:

See PR instructions doc for full pull request review instructions.

@cantsin
Copy link
Contributor Author

cantsin commented Jan 5, 2021

@Jkrzy flagging you as a reviewer too -- oddly enough Github is not populating your username in the reviewers field for whatever reason.

@cantsin
Copy link
Contributor Author

cantsin commented Jan 5, 2021

Anyway, I'd like one of y'all to go through the documentation and let me know what worked, what was unclear, what could be improved. Thanks!

@Jkrzy
Copy link
Collaborator

Jkrzy commented Jan 7, 2021

This is :amaze:ing @cantsin!!!

Running through the docs I get tripped up on collectstatic with:

s3_1 | 2021-01-07T15:23:09:INFO:localstack.services.edge: Unable to find forwarding rule for host "s3:4566"

and this exception from boto: botocore.exceptions.ClientError: An error occurred (404) when calling the PutObject operation: Not Found

Did you hit this in local testing?

@cantsin cantsin requested a review from Jkrzy January 7, 2021 17:54
@cantsin
Copy link
Contributor Author

cantsin commented Jan 7, 2021

@Jkrzy I have not gotten that error! To double check the obvious, did you rebuild the docker-compose image? I just ran through the steps again myself and so far I don't see that error.

  • Does http://localhost:4566/health?reload show the s3 service running?
  • If you do docker-compose run web curl http://s3:4566/health what shows up?

These two test external and internal access to the localstack edge port, respectively.

@cantsin
Copy link
Contributor Author

cantsin commented Jan 11, 2021

@Jkrzy updated to address the issue we found last Friday. Please note that you will need to remove your local S3 bucket and re-create it (since collectstatic will skip if the file is already on S3 and therefore not save to staticfiles in the container as per our new storage class). Alternatively, I think removing the files: aws s3 rm --recursive s3://crt-portal/ --endpoint-url=http://localhost:4566 will also work.

Tested this time by doing git clean -fxd beforehand 😀

Copy link
Collaborator

@Jkrzy Jkrzy left a comment

Choose a reason for hiding this comment

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

Thanks @cantsin! We're super close but this latest change reverts to the non-manifesting storage implementation.

crt_portal/crt_portal/storage.py Outdated Show resolved Hide resolved
crt_portal/crt_portal/storage.py Outdated Show resolved Hide resolved
cantsin and others added 3 commits January 12, 2021 09:28
compressor redux: use S3ManifestStaticStorage instead

Co-authored-by: Joe Krzystan <joseph.krzystan@gsa.gov>
compressor redux: fix super

Co-authored-by: Joe Krzystan <joseph.krzystan@gsa.gov>
@cantsin cantsin requested a review from Jkrzy January 12, 2021 18:49
Copy link
Collaborator

@Jkrzy Jkrzy left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for sticking with this one @cantsin!

@Jkrzy Jkrzy merged commit ffb5f87 into develop Jan 13, 2021
@Jkrzy Jkrzy deleted the compressor-redux branch January 13, 2021 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants