-
Notifications
You must be signed in to change notification settings - Fork 3
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 reserved ip submodule #415
base: main
Are you sure you want to change the base?
Conversation
/run pipeline |
5 similar comments
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
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.
Hi Jordan, a couple of comments inline. Also the top level readme should have one or 2 sentences around the reserved ip feature and why it exists.
/run pipeline |
1 similar comment
/run pipeline |
blocked by #435 |
/run pipeline |
8 similar comments
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
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 is an upgrade test in other_test.go. These tests are run during the continuous test, however, in that context what is the upgrade?
/run pipeline |
2 similar comments
/run pipeline |
/run pipeline |
/run pipeline |
1 similar comment
/run pipeline |
] | ||
subnet_zone_list = [ | ||
[ | ||
{ |
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 there a reason for having to pass redundant data for the existing subnets that are targeted?
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.
modules/reserved-ips/README.md
Outdated
} | ||
``` | ||
|
||
The above will create 6 new reserved ip's as such and output them: |
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.
Will that example really create 6 ips or just 3 given that 3 subnets are passed.
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.
3 for each service passed in cloud_services
.
variable "subnet_zone_list" { | ||
description = "List of subnets in the VPC where gateways and reserved IPs will be provisioned. This value is intended to use the `subnet_zone_list` output from the Landing Zone VPC Subnet Module (https://github.com/terraform-ibm-modules/terraform-ibm-landing-zone-vpc) or from templates using that module for subnet creation." | ||
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.
Do we really need to ask for all of those informations?
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 could remove cidr
, need others for naming convention.
modules/reserved-ips/variables.tf
Outdated
default = "us-south" | ||
} | ||
|
||
variable "cloud_services" { |
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.
It's only used for the naming of the reserved ips, so I would make this field more generic to take a list of strings. Also the description is not accurate.
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.
vpe_name
maybe needed in naming convention to match main module.
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
Description
#395
Release required?
x.x.X
)x.X.x
)X.x.x
)Release notes content
Provides a way to create a reserved ip independently from the VPE creation. Ensures that the consumer has got full control on the reserved ip's being attached. This is used in the scenario where a consumer wants to have the same ip for a given VPE type across VPC's.
Run the pipeline
If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.
Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:
Checklist for reviewers
For mergers