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

Jump to bottom

fix(aws-cloudfront): Fix loggingConfig being ignored #805

Closed
wants to merge 8 commits into from
Closed

fix(aws-cloudfront): Fix loggingConfig being ignored #805

wants to merge 8 commits into from

Conversation

ZeldoKavira
Copy link
Contributor

@ZeldoKavira ZeldoKavira commented Sep 28, 2018 •
edited
Loading

feat(aws-s3): Add support for domainName on BucketRef instead of just Bucket

Fixes #721

Note: I know this goes against the policy of one fix/feature/ect per PR, but the s3 feature was required to complete this PR.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

fix(aws-cloudfront): Fix loggingConfig being ignored
ac09b96 
feat(aws-s3): Add support for domainName on BucketRef instead of just Bucket

LoggingConfiguration now requires bucket instead of it being optional. There is no point to including a LoggingConfiguration without a bucket (no-breaking).

Fixes aws#721
@ZeldoKavira
Copy link
Contributor Author

@eladb || @rix0rrr Cannot set a reviewer

@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 28, 2018

I was ready to approve this, but while fixing up the docstrings I noticed that the intended purpose of bucket being optional was that a logging bucket would be automatically created.

That still seems like desirable behavior to me, so can you make that work? It involves new'ing up an s3.Bucket() on the spot if it happens that the bucket is unset. Probably makes sense to expose loggingBucket?: s3.BucketRef as a public attribute as well, so consumers can access the bucket object afterwards.

@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 28, 2018

Oh and apparently I broke something. Sorry! 👀

@ZeldoKavira
Copy link
Contributor Author

I will take a look at the auto generation!

rix0rrr and others added 2 commits September 28, 2018 19:53
@rix0rrr
Fix whitespace errors
cb52b3a
Fix whitespace errors with S3
24723ed 
Auto generate logging bucket if one is not provided + tests
@ZeldoKavira
Copy link
Contributor Author

ZeldoKavira commented Sep 28, 2018 •
edited
Loading

@rix0rrr Lets pause this one for a second, found a potential issue with CloudFormation

@ZeldoKavira
Copy link
Contributor Author

@rix0rrr Resolved the issue.

@ZeldoKavira
Copy link
Contributor Author

ZeldoKavira commented Sep 28, 2018 •
edited
Loading

@rix0rrr Fixed merge issue from the other PR being accepted, if possible can you cut a release after this PR is accepted?

@ZeldoKavira
Copy link
Contributor Author

I messed up and overlooked export(), reverting to a different branch to keep the commits clean

@ZeldoKavira ZeldoKavira mentioned this pull request Sep 28, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Logging property not present on creating a Cloudfront distribution using construct
2 participants
@ZeldoKavira @rix0rrr