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

adding support to filter on 'vpc attribute' #1908

Merged
merged 5 commits into from
Jan 4, 2018

Conversation

joshuaroot
Copy link
Contributor

Per #1616 , adding support to filter VPC based on vpc-attribute


for r in resources:
hostname = client.describe_vpc_attribute(
VpcId=r['VpcId'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally only fetch the attribute if it’s being matched on

"""Filters VPCs based on their DNS attributes"""
schema = type_schema(
'vpc-attributes',
EnableDnsHostnames={'type': 'boolean'},
Copy link
Collaborator

Choose a reason for hiding this comment

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

We be generally gone for lower case hyphenated when not letting the user specify arbitrary keys

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha. I had originally used lower case for the parameters but noted a few instances where upper case was used.

@joshuaroot
Copy link
Contributor Author

I also forgot to add in a mock policy in the docstring...

- corrected parameter case (all lower case)
- added example policy in doctstring
- added second unittest for single attribute matching
@kapilt
Copy link
Collaborator

kapilt commented Dec 29, 2017

i'd suggest we also drop the 'enable' prefix on the attributes.

- removing 'enabled' prefix from parameters
- correcting example policy to make sense with provided filter
- correcting unittest (previously changed parameter syntax)
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, lgtm

@kapilt kapilt merged commit 28f52d0 into cloud-custodian:master Jan 4, 2018
lamyanba pushed a commit to lamyanba/cloud-custodian that referenced this pull request Apr 23, 2019
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.

2 participants