-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(route53resolver): improve FirewallRuleGroup import by name #33788
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
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Leo10Gama
left a comment
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.
Thanks for your contribution and patience! This seems like a great addition to help this alpha module come together. I've left a few outstanding comments of things we should tweak to improve this PR. Happy to approve of these changes once they're addressed!
| const firewallRuleGroupId = firewallRuleGroups[firewallRuleGroupName]; | ||
|
|
||
| if (!firewallRuleGroupId) { | ||
| throw new Error(`Firewall Rule Group with name "${firewallRuleGroupName}" not found in context.`); |
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.
Can we make this a ValidationError instead?
| this.node.setContext('firewallRuleGroups', { | ||
| TestRuleGroup: 'fwr-12345678', | ||
| }); |
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 not sure I agree with this structure for the integration tests. We typically want these tests to reflect the way people would use the CDK in the wild, so it would probably be better to have the construct declared independently, and then imported using the new method from another stack.
Looking through the repository, it seems like this integ test is checking similar functionality in a cleaner structure. Could we replace this with a similar structure?
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.
Let's add an assertion to this integ test as well, for good measure. This integ test is a pretty good example of how we might go about doing this, using AWS API to check the values that are deployed when the test is run.
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 14 days if no action is taken. |
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue
Closes #16335.
Reason for this change
Currently,
FirewallRuleGroupallows importing an existing rule group by ID usingfromFirewallRuleGroupID(), but there is no built-in way to reference an existing rule group by name. Users need to implement their own lookup logic via the AWS SDK. This change provides a direct method to import a firewall rule group by name, simplifying the process.Description of changes
FirewallRuleGroup.fromFirewallRuleGroupName(), which allows users to import a firewall rule group using its name instead of just an ID.Describe any new or updated permissions being added
None
Description of how you validated changes
Checklist
Acknowledgements
Thanks to @arvchahal for collaboration on this feature!
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license