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

Add dry_run and fix s3 backend merging behaviour #50

Merged
merged 16 commits into from
Mar 8, 2024

Conversation

lakkeger
Copy link
Contributor

@lakkeger lakkeger commented Feb 29, 2024

Motivation

  • a frequently requested feature from users to be able to generate the override file for debugging purposes
  • noticed by some users that the s3 backend endpoints generation was faulty

Dry run

  • set via DRY_RUN env variable
  • if unset:
    • if the override file exists, tflocal throws an exception
    • if the override file not existing, tflocal creates override file and calls Terraform
  • if set:
    • if the override file not existing, tflocal generates the override file and exits
    • if the override file exists, prompts user to confirm overwrite and only then generates override file

S3 backend endpoints behaviour

Old behaviour

  1. if endpoints block defined, uses that even if its incomplete
  2. if legacy endpoint options (endpoint,sts_endpoint...etc) are present in the original file and Terraform >v1.5 it ignores these options
  3. if none of the above are present generates the right endpoints config for the used Terraform version

New behaviour

Terraform version >1.5:

  1. if endpoints is present:
    1. and complete, uses that
    2. and incomplete:
      1. and legacy options are present, adds legacy options and then the missing defaults
      2. and legacy options are not present, adds the missing defaults
  2. if legacy options are present:
    1. and no endpoints set, uses 1.ii.a. behaviour
    2. and endpoints are set, see behaviour at endpoints

Terraform version <=1.5:

  • only the legacy options accepted, if any of them missing we set the defaults
  • for new options we throw exception and exit

From the created data structure according to the Terraform version generates the right s3 backend config:

  • Terraform version >1.5, endpoints block
  • Terraform version <=1.5, legacy options

Changes

  • add DRY_RUN env variable
  • add dry run behaviour
  • early check for override file
  • fix s3 backend endpoints behaviour
  • fix s3 backend legacy options merging
  • add tests for s3 backend options
  • add tests for dry run

@lakkeger lakkeger requested a review from whummer March 5, 2024 16:18
@lakkeger lakkeger self-assigned this Mar 5, 2024
@lakkeger lakkeger marked this pull request as ready for review March 5, 2024 16:18
@lakkeger lakkeger added enhancement New feature or request bug Something isn't working labels Mar 5, 2024
Copy link
Member

@whummer whummer left a comment

Choose a reason for hiding this comment

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

Kudos for tackling this @lakkeger ! Great to see that we're addressing an often requested feature from our users 🚀

I have a few minor nitpick comments, and one more fundamental question related to comparing the file hashes in the tests..

bin/tflocal Outdated Show resolved Hide resolved
bin/tflocal Outdated Show resolved Hide resolved
bin/tflocal Outdated Show resolved Hide resolved
bin/tflocal Outdated Show resolved Hide resolved
tests/test_apply.py Outdated Show resolved Hide resolved
tests/test_apply.py Outdated Show resolved Hide resolved
bin/tflocal Outdated Show resolved Hide resolved
Copy link
Member

@whummer whummer left a comment

Choose a reason for hiding this comment

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

Kudos for the quick turnaround and addressing all the comments @lakkeger ! 🚀

except Exception as e:
print(f'Unable to parse "{override_file}" as HCL file: {e}')

new_options_check = "endpoints" in result and all(map(lambda x: x in result.get("endpoints"), new_options))
Copy link
Member

@whummer whummer Mar 6, 2024

Choose a reason for hiding this comment

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

Nice! That's great - seems much cleaner imo (and more forward-compatible) to parse the file and explicitly check for the contents we're expecting. 👌 (as compared to the file hashing)

@lakkeger lakkeger merged commit be1f665 into main Mar 8, 2024
3 checks passed
@lakkeger lakkeger deleted the add_dry_run_and_fix_s3_backend_configs branch March 8, 2024 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants