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

[codepipeline-actions] max-age header not set correctly for s3 deploy action #8774

Closed
nonken opened this issue Jun 28, 2020 · 5 comments · Fixed by #8864
Closed

[codepipeline-actions] max-age header not set correctly for s3 deploy action #8774

nonken opened this issue Jun 28, 2020 · 5 comments · Fixed by #8864
Assignees
Labels
@aws-cdk/aws-codepipeline-actions bug This issue is a bug. in-progress This issue is being actively worked on. p1

Comments

@nonken
Copy link
Contributor

nonken commented Jun 28, 2020

The Cache-Control max-age key value gets set wrongly when using an S3DeploymentAction.

Current max-age: Number
Expected max-age=Number

Thanks to the excellent and super fast triage from @mikesherov 🙏.

https://twitter.com/mikesherov/status/1277320422791528457

The faulty line can be found here:

public static maxAge(t: Duration) { return new CacheControl(`max-age: ${t.toSeconds()}`); }

The same applies for the s-maxage property:

public static sMaxAge(t: Duration) { return new CacheControl(`s-maxage: ${t.toSeconds()}`); }

Also see here https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control

Reproduction Steps

new S3DeployAction({
  cacheControl: [CacheControl.maxAge(Duration.days(365))]
});

Environment

  • CLI Version : 1.47.0
  • Framework Version:
  • Node.js Version:
  • OS :
  • Language (Version):

Other


This is 🐛 Bug Report

@nonken nonken added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 28, 2020
@SomayaB SomayaB changed the title [aws-s3-deployment.CacheControl] max-age header not set correctly [aws-s3-deployment] max-age header not set correctly Jun 29, 2020
@skinny85
Copy link
Contributor

Thanks for opening the issue @nonken !

Any chance you can open us a PR fixing this? https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md#getting-started 😃

@nonken
Copy link
Contributor Author

nonken commented Jun 29, 2020

@skinny85 I should be able to get to this sometime this week yep. Especially for a former colleague 🤗

@nonken
Copy link
Contributor Author

nonken commented Jul 7, 2020

@skinny85 I have opened a PR, would be amazing if this can go in as it is breaking the interwebs 😄

@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Jul 7, 2020
@skinny85
Copy link
Contributor

skinny85 commented Jul 7, 2020

@skinny85 I have opened a PR, would be amazing if this can go in as it is breaking the interwebs 😄

Approved - I don't want to keep the interwebs waiting! 😅

Thanks so much for taking care of this issue @nonken !

@mergify mergify bot closed this as completed in #8864 Jul 7, 2020
mergify bot pushed a commit that referenced this issue Jul 7, 2020
…he control (#8864)

The `max-age` and `s-maxage` header properties were assigned using a `:` instead of `=`. This change fixes the assignment to be according to the spec. Also added missing test for the `s-maxage` property.

Fixes #8774


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@iliapolo iliapolo changed the title [aws-s3-deployment] max-age header not set correctly [codepipeline] max-age header not set correctly Jul 8, 2020
@iliapolo iliapolo changed the title [codepipeline] max-age header not set correctly [codepipeline] max-age header not set correctly for s3 deploy action Jul 8, 2020
@github-actions github-actions bot added the @aws-cdk/aws-codepipeline Related to AWS CodePipeline label Jul 8, 2020
@nonken
Copy link
Contributor Author

nonken commented Jul 8, 2020

🤗 Wohoo!!

@iliapolo iliapolo changed the title [codepipeline] max-age header not set correctly for s3 deploy action [codepipeline-actions] max-age header not set correctly for s3 deploy action Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-codepipeline-actions bug This issue is a bug. in-progress This issue is being actively worked on. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants