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

chore: update perimeter_additional_members instructions #1038

Merged

Conversation

romanini-ciandt
Copy link
Member

@romanini-ciandt romanini-ciandt commented Nov 24, 2023

Issue #936

This PR implements an update on perimeter_additional_members variable declaration in both 3-networks steps.

Basically, we want to avoid a situation where the user forget to uncomment a .tfvars line and because of that don't have access to a project protected by the VPC service controls. With this change, if the user forgets to fulfil the variable, an error will be raised during deploy process.

Copy link
Collaborator

@rjerrems rjerrems left a comment

Choose a reason for hiding this comment

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

Rather than providing an uncommented bogus parameter, why don't we make it so that perimeter_additional_members has no default value, so that it errors out when not provided?

In that case, if a user does not want to provide any members they can add [""]

@romanini-ciandt
Copy link
Member Author

romanini-ciandt commented Nov 27, 2023

@rjerrems thanks for your inputs!
In order to solve the #936 issue, I'm implementing a dummy value for perimeter_additional_members variable on .tfvars that should be replaced like we have for others required variables (i.e: in the same file we have example.com. for domain)

Nowadays, perimeter_additional_members variable doesn't have a default in variables.tf, so the current behavior is to have an error when not provided. The goal of this PR is to standardize the required variable's declaration in .tfvars and make all of them (the required ones) uncommented.

With this context, do you think that makes sense to keep this PR as is?

Copy link
Collaborator

@rjerrems rjerrems left a comment

Choose a reason for hiding this comment

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

Ok thanks for the context - probably worth checking with @eeaton before merging to make sure it addresses the issue fully.

@eeaton
Copy link
Collaborator

eeaton commented Nov 28, 2023

This works for me. At a future date I'm planning on doing a more thorough review of the directions in README files for this type of usability challenge, but for the scope of issue #936 that I raised, this works.

@daniel-cit
Copy link
Contributor

@gtsorbo this pull request edits README.md markdown files and common.auto.example.tfvars files that are not used in the integration build process.

@gtsorbo gtsorbo merged commit fc7cc6b into terraform-google-modules:master Dec 12, 2023
5 checks passed
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.

documentation: A mandatory variable perimeter_additional_members is commented out
5 participants