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

Add origin_response lambda function #16

Merged
merged 1 commit into from Mar 27, 2020

Conversation

crungehottman
Copy link
Member

The cache_control function appends a "Cache-Control: max_age=600" header to HTTP responses from the origin server.

Resolves: DELIVERY-8986

@rohanpm
Copy link
Member

rohanpm commented Mar 19, 2020

I don't think it should be named cache_control, should it? Because we expect that we will need to add some more behaviors onto origin-response events, and we'll need all of that to be handled by one lambda function, so we would end up with a function named cache_control which is actually doing a bunch more.

Actually, the internal name doesn't matter much, but could we please have an entry for the function in cdn_lambda/__init__.py named origin_response to match the existing origin_request?

@negillett
Copy link
Member

negillett commented Mar 19, 2020

@rohanpm, @crungehottman, I wouldn't be opposed to naming the function "origin_response" and changing "map_to_s3" to "origin_request". I don't foresee swapping out request or response functions on our distributions, but rather adding functionality to the existing functions as needed.

@crungehottman crungehottman changed the title Add cache_control lambda function Add origin_response lambda function Mar 20, 2020
"status": "404",
"statusDescription": "Not Found",
}
return {"status": "404", "statusDescription": "Not Found"}
Copy link
Member Author

Choose a reason for hiding this comment

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

note: black reformatted this line

cdn_lambda/functions/map_to_s3/map_to_s3.py Outdated Show resolved Hide resolved
cdn_lambda/functions/origin_response/origin_response.py Outdated Show resolved Hide resolved
cdn_lambda/functions/origin_response/origin_response.py Outdated Show resolved Hide resolved
cdn_lambda/functions/origin_response/origin_response.py Outdated Show resolved Hide resolved
cdn_lambda/functions/origin_response/origin_response.py Outdated Show resolved Hide resolved
tests/functions/test_origin_response.py Outdated Show resolved Hide resolved
tests/functions/test_origin_response.py Outdated Show resolved Hide resolved
tests/functions/test_origin_response.py Outdated Show resolved Hide resolved
@crungehottman crungehottman force-pushed the cache-control branch 2 times, most recently from 82d3783 to 48979dd Compare March 24, 2020 19:27
".+/PULP_MANIFEST",
".+/listing",
".+/repodata/repomd.xml",
".+/ostree/repo/refs/heads/.*/.*",
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a regex newbie -- I don't think this pattern is optimal, but it works. Feel free to suggest a more optimal regex.

Copy link
Member

@rohanpm rohanpm left a comment

Choose a reason for hiding this comment

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

I think this will break "tox -e package" because the config file path is written there (in tox.ini), could you please have a look?

I'm not sure how you generate your deployment packages right now during development but you should maybe look into using that command if you aren't already, ideally we are all using the same method to make a deployment package so we will find the same issues.

CI also really ought to be testing that tox command, I'll make a separate PR for that.

cdn_lambda/functions/map_to_s3/map_to_s3.py Outdated Show resolved Hide resolved
cdn_lambda/functions/origin_response/origin_response.py Outdated Show resolved Hide resolved
@crungehottman
Copy link
Member Author

I think this will break "tox -e package" because the config file path is written there (in tox.ini), could you please have a look?

It did break tox - e package. I updated the path to the config file in tox.ini and now tox -e package is passing.

I'm not sure how you generate your deployment packages right now during development but you should maybe look into using that command if you aren't already, ideally we are all using the same method to make a deployment package so we will find the same issues.

I have not been generating deployment packages, but I will be sure to include tox -e package in my development workflow.

CI also really ought to be testing that tox command, I'll make a separate PR for that.

Thanks!

@crungehottman crungehottman marked this pull request as ready for review March 26, 2020 13:11
Copy link
Member

@rohanpm rohanpm left a comment

Choose a reason for hiding this comment

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

github says there are conflicts, would you mind fixing those too for the next round of review?

cdn_lambda/functions/origin_response/origin_response.py Outdated Show resolved Hide resolved
Copy link
Member

@rohanpm rohanpm left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

There are still conflicts though. I'll resolve them on your behalf later if needed.

The origin_response function appends a configurable "Cache-Control: max_age=<value>" header to a subset of HTTP responses from the origin server.

Resolves: DELIVERY-8986
@rohanpm
Copy link
Member

rohanpm commented Mar 27, 2020

Resolved the conflicts. I'll submit this for you now so it lands prior to repo rename.

@rohanpm rohanpm merged commit 484768c into release-engineering:master Mar 27, 2020
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

3 participants