-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: Add dedicated custom firewall rules module #200
Conversation
@morgante @bharathkkb my test seems to be hitting Any recommendation on this? The test seems unrelated to mine, but seems like there might be too many networks being created in the project. Should I merge the firewalls test with another test? |
Hi @umairidris |
Thanks Bharath. I'll go with the merge approach for now. |
So I added the checks under secondary ranges, but
|
@umairidris that's odd last commit to main seems green, I will take a look. |
variable "rules" { | ||
description = "List of custom rule definitions (refer to variables file for syntax)." | ||
default = [] | ||
type = list(object({ |
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 see that we require object
here but in the main module take in any
and convert into this list of objects for this submodule? Is there a reason why we just take in any
here and and do the defaults lookup here?
I am thinking if users want to use this submod outside of main module, they will need to define everything since its a list of objects.
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.
Ideally we should use list of objects everywhere. However as you pointed out users will need to define all variables possible through this. I chose to make the submodule list of objects to increase type checking while temporarily keeping the main module as type any
until Terraform supports object defaults.
I thought that was a decent compromise (if someone wants type checking they can use submodule but if they want a simpler interface then the main module implements that). Also, I was going to implement both as list of objects, but then the firewall rules would have a very different interface compared to the other fields such as subnets.
I am ok with whatever you guys suggest. If you want any
for both levels then I can make the change.
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.
On further thought, I think if users want to ignore type checking they can just wrap the fw submodule like we do with main module. On a side note, I don't think defaults are slated till 0.14.0
😞
@umairidris looks like we have not pinned the CI images yet maybe thats why. Could you try pinning both images in lint and int CB yamls to |
/gcbrun |
Success! Thanks @bharathkkb. I still saw the failure for the test come up once more, but running /gcbrun to retry seems to have passed it, so it seems that specific test is a bit flaky for whatever reason. |
/gcbrun |
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.
Overall LGTM
Just a question
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days |
@umairidris Sorry we left this sitting for a while, do you mind resolving conflicts so we can merge? |
Thanks for the PR! 🚀 |
@morgante done. |
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 patience. 💯
*/ | ||
|
||
terraform { | ||
required_version = ">=0.12.6, <0.14" |
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.
is this is a breaking change as module version 3.0 didn't require <0.14 and now 3.1 does? I was on module version 3.0 and TF 0.14.5. In order to update to module version 3.1, I must downgrade the TF version
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.
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.
and i just saw your pr for the fix (#245) ignore me.
fyi : #247 |
#199