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

AWS - streaming_distribution set-attributes action and config filter #5694

Merged

Conversation

Lucas-Irvine
Copy link
Contributor

Add a set-attributes action for streaming distributions, as well as a streaming-distribution-config filter. I also included two bug fixes for regular Cloudfront distributions: one where I wasn't properly using the distribution-config filter in the tests, and the other where I didn't set the 'Comment' and 'Enabled' required fields if they were not included in the attributes part of the policy.

Copy link
Collaborator

@kapilt kapilt left a comment

Choose a reason for hiding this comment

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

there should be more opportunities here to share common implementation between the distribution and streaming distribution implementations. please refactor to a base class to minimize the duplication.

c7n/resources/cloudfront.py Outdated Show resolved Hide resolved
@Lucas-Irvine
Copy link
Contributor Author

Hi Kapil, can you expand on what you mean by refactor to a base class / what exactly shouldn't be duplicated? The required fields in update_streaming_distribution() and update_distribution() are different as well as the exceptions they throw. Also distribution and streaming-distribution were already classified as different resources, so I'm not sure on how to more refactor the code. Thanks!

@kapilt
Copy link
Collaborator

kapilt commented May 1, 2020

Hi Kapil, can you expand on what you mean by refactor to a base class / what exactly shouldn't be duplicated? The required fields in update_streaming_distribution() and update_distribution() are different as well as the exceptions they throw. Also distribution and streaming-distribution were already classified as different resources, so I'm not sure on how to more refactor the code. Thanks!

parameterize the parts that are different, and let the base class use that to do the legwork, subclasses just become setting class attributes. also on name this should be distribution-config, its already qualified to the streaming distribution, so the prefix is redundant.

c7n/resources/cloudfront.py Outdated Show resolved Hide resolved
@Lucas-Irvine
Copy link
Contributor Author

Ok, I tried refactoring the distribution-config filter and the set-attributes action to a base class as you said. Let me know if I should also factor out the augment and process_distribution methods - I thought they looked kind of messy taking 5 or 6 arguments in the base class and I didn't know how to properly pass the client methods without creating duplicate clients. I also made the required fields to be a class attribute as you suggested.

@Lucas-Irvine Lucas-Irvine requested a review from kapilt May 8, 2020 16:01
c7n/resources/cloudfront.py Outdated Show resolved Hide resolved
@Lucas-Irvine Lucas-Irvine requested a review from kapilt May 25, 2020 01:21
Copy link
Collaborator

@kapilt kapilt left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, lgtm

@kapilt kapilt merged commit 91a8a16 into cloud-custodian:master May 28, 2020
fidelito pushed a commit to fidelito/cloud-custodian that referenced this pull request May 29, 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

2 participants