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

fix(caf_solution/locals.remote_tfstates.tf): changing order of global… #448

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nusrath432
Copy link

@nusrath432 nusrath432 commented Feb 17, 2023

…_settings within the merge function

  • when reading global_settings the current landingzone global_settings block should be given preference. Since a merge function is used, the order of items in the map are important. - global_settings from reference tfstate files should only be included if global_settings_key = is defined. - hence, reading global_settings from first tfstate is removed. try(data.terraform_remote_state.remote[keys(var.landingzone.tfstates)[0]].outputs.global_settings, null), - updated tags - tags defined in global_settings {} or custom_variables {} must have highest precedence.

Issue-id: 403

PR Checklist


  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My code follows the code style of this project.
  • I ran lint checks locally prior to submission.
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Description

When defining global_settings block for a given landingzone, the current landingzone should take precedence over global_settings defined in lower/other current landingzones. Also, the global_settings should only be imported when global_settings_key is defined. Since a merge function is used, the order of items in the map are important.

  • when reading global_settings the current landingzone global_settings block should be given preference. Since a merge function is used, the order of items in the map are important.
  • global_settings from reference tfstate files should only be included if global_settings_key = is defined.
  • hence, reading global_settings from first tfstate is removed.
    try(data.terraform_remote_state.remote[keys(var.landingzone.tfstates)[0]].outputs.global_settings, null),

Does this introduce a breaking change

  • [X ] YES [Potentially]
  • NO

Testing

Create a landingzone with global_settings [passthrough = true;]
Create another landingzone with global_settings [passthrough = false; prefix=xyz]
Now withing the landingzone.tfvars, reference the 1st landingzone within the tfstate block.

…_settings within the merge function

- when reading global_settings the current landingzone global_settings
    block should be given preference. Since a merge function is used, the
    order of items in the map are important.
    - global_settings from reference tfstate files should only be included
      if global_settings_key = <landingzone-key> is defined.
    - hence, reading global_settings from first tfstate is removed.
    try(data.terraform_remote_state.remote[keys(var.landingzone.tfstates)[0]].outputs.global_settings, null),
    - updated tags - tags defined in global_settings {} or custom_variables {} must have highest precedence.
@arnaudlh arnaudlh added bug Something isn't working enhancement New feature or request and removed enhancement New feature or request labels Mar 22, 2023
try(data.terraform_remote_state.remote[var.landingzone.global_settings_key].outputs.objects[var.landingzone.global_settings_key].global_settings, null),
try(data.terraform_remote_state.remote[var.landingzone.global_settings_key].outputs.global_settings, null),
try(data.terraform_remote_state.remote[keys(var.landingzone.tfstates)[0]].outputs.global_settings, null),
Copy link
Contributor

Choose a reason for hiding this comment

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

This entry was put for backward compatibility as originally the global_setting_key was not always defined, so we took the global settings from the tfstate in the map.
Why not keeping it?

I think it is a good idea to add the var.global_settings as it will override any previous settings. Good spot

Copy link
Author

Choose a reason for hiding this comment

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

@LaurentLesle Lets say we have the following use-case:

  1. Lower Landingzones has global_settings { passthrough = true } and we deployed all the resources in that landingzone without any prefix, these are shared resources hence no prefix.

  2. Then I define two new higher landingzones (same level) one for prod with global_settings = { prefix=prod} and another for non-prod with global_settings = { prefix=nonprod }

  3. Now when I reference any lower level resource in my prod/nonprod spoke landingzones, my expectation is to just use tfstate to reference lower level landingzone objects but not inherit its global_settings unless explicitly intended by defining a "global_settings_key = "mylowerlevel-landingzone"". If not, the moment we define global_settings_key, which does not have any prefix set, Terraform would start deleting resources defined with prefix=prod and prefix=nonprod

  4. The overriding with var.global_settings may not work due to the merge function order.
    "If more than one given map or object defines the same key or attribute, then the one that is later in the argument sequence takes precedence. " Ref: https://developer.hashicorp.com/terraform/language/functions/merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants