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

revisit prefix handling #954

Closed
juliocc opened this issue Nov 7, 2022 · 4 comments · Fixed by #964
Closed

revisit prefix handling #954

juliocc opened this issue Nov 7, 2022 · 4 comments · Fixed by #964
Assignees

Comments

@juliocc
Copy link
Collaborator

juliocc commented Nov 7, 2022

Handling of the prefix variable is inconsistent across modules. For example project doesn't allow "" but gcs does.

Agree on a single pattern and use it in all modules.

@skalolazka skalolazka self-assigned this Nov 10, 2022
@skalolazka
Copy link
Member

To summarize the proper usage:

  1. Conditional in locals to detect null and empty string
  2. Default null in variables.tf
    So the ideal way is, for example, in the iam-service-account module:
    prefix = var.prefix == null || var.prefix == "" ? "" : "${var.prefix}-"
    Is my understanding correct?

@juliocc
Copy link
Collaborator Author

juliocc commented Nov 10, 2022

I would prefer validating that prefix != "" and just handling the case when prefix == null

The local for iam-service-account would end up like this:

prefix = var.prefix == null ? "" : "${var.prefix}-"

@ludoo wdyt?

@ludoo
Copy link
Collaborator

ludoo commented Nov 10, 2022

I would prefer validating that prefix != "" and just handling the case when prefix == null

The local for iam-service-account would end up like this:

prefix = var.prefix == null ? "" : "${var.prefix}-"

@ludoo wdyt?

yes, absolutely. Validation in the variable for empty string, and test on null in locals.

@skalolazka
Copy link
Member

Opened #967 for the blueprints.

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 a pull request may close this issue.

3 participants