-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add origin_response lambda function #16
Conversation
4bc9630
to
12893e3
Compare
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 |
@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. |
12893e3
to
b6f7462
Compare
"status": "404", | ||
"statusDescription": "Not Found", | ||
} | ||
return {"status": "404", "statusDescription": "Not Found"} |
There was a problem hiding this comment.
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
b6f7462
to
c408a2d
Compare
82d3783
to
48979dd
Compare
".+/PULP_MANIFEST", | ||
".+/listing", | ||
".+/repodata/repomd.xml", | ||
".+/ostree/repo/refs/heads/.*/.*", |
There was a problem hiding this comment.
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.
48979dd
to
28cde47
Compare
There was a problem hiding this 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.
28cde47
to
1872222
Compare
It did break
I have not been generating deployment packages, but I will be sure to include
Thanks! |
There was a problem hiding this 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?
1872222
to
dfcd772
Compare
There was a problem hiding this 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
Resolved the conflicts. I'll submit this for you now so it lands prior to repo rename. |
The cache_control function appends a "Cache-Control: max_age=600" header to HTTP responses from the origin server.
Resolves: DELIVERY-8986