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

feat: adds --skip-init flag #193

Merged
merged 1 commit into from Sep 10, 2022
Merged

feat: adds --skip-init flag #193

merged 1 commit into from Sep 10, 2022

Conversation

Gowiem
Copy link
Member

@Gowiem Gowiem commented Sep 7, 2022

what

  • Adds a --skip-init flag which allows skipping terraform init

why

  • This can help speed up workflows in the case that the user knows their last command successfully ran terraform init and they do not need to run init again.

references

@Gowiem
Copy link
Member Author

Gowiem commented Sep 7, 2022

Hey @aknysh -- this isn't ready because I have an issue that I'd like to get your thoughts on: When passing --skip-init, this works successfully to skip over the terraform init process, but unfortunately atmos then passes the --skip-init flag down to terraform via Arguments and flags: [--skip-init], which we don't want.

To clarify, the following happens:

  1. I run the following: /localhost/Workspace/Contracting/cp/atmos/build/atmos terraform plan monitor -s dev --skip-init
  2. It skips over terraform init and prints the correct "Skipping over terraform init due to --skip-init being passed." message.
  3. It then goes to execute terraform plan and errors with the following:
Executing command:
/usr/bin/terraform plan -var-file dev-monitor.terraform.tfvars.json -out dev-monitor.planfile --skip-init

│ Error: Failed to parse command-line flags
│
│ flag provided but not defined: -skip-init

So couple questions here:

  1. How do you deal with this for --dry-run? Is there some other way I should be going about implementing this?
  2. Any suggestions on how I could test this functionality?

@Gowiem Gowiem requested a review from osterman September 7, 2022 23:45
@aknysh
Copy link
Member

aknysh commented Sep 8, 2022

@Gowiem thanks for this PR.

See https://github.com/cloudposse/atmos/blob/master/internal/exec/utils.go#L16

commonFlags are a list of flags that atmos understands but not terraform/helmfile etc.
The flags get removed from the arg list after atmos uses it and then passes them down to terraform/helmfile.

You need to add your flag to the list.
And please add a comment for the var to explain what I just explained.

To test it:

From the root of the repo, you can execute any atmos commands like this:

go run github.com/cloudposse/atmos help
go run github.com/cloudposse/atmos describe stacks
go run github.com/cloudposse/atmos terraform plan test-component  -s tenant1-ue2-dev

It uses the component and stacks from the examples/complete.

To test your flag:

go run github.com/cloudposse/atmos terraform plan test/test-component  -s tenant1-ue2-dev --skip-init

Thanks

@Gowiem
Copy link
Member Author

Gowiem commented Sep 8, 2022

@aknysh Ah good stuff on commonFlags. Thanks for the pointer!

Regarding testing, I was able to build in a client toolbox image and test it out. I'm more interested to know if you'd like unit tests and if you have any suggestions for unit testing it.

@aknysh
Copy link
Member

aknysh commented Sep 8, 2022

@aknysh Ah good stuff on commonFlags. Thanks for the pointer!

Regarding testing, I was able to build in a client toolbox image and test it out. I'm more interested to know if you'd like unit tests and if you have any suggestions for unit testing it.

not sure about what unit testing would do (please explain), but we have a ton of tests in the repo, we try to add tests for every new feature we introduce. Go runs all of them automatically

fix: adds SkipInitFlag to commonFlags so as to not pass down to command

docs: adds --skip-init to `atmos terraform --help` docs
@Gowiem Gowiem marked this pull request as ready for review September 9, 2022 18:10
@Gowiem Gowiem requested review from a team as code owners September 9, 2022 18:10
@Gowiem
Copy link
Member Author

Gowiem commented Sep 9, 2022

@aknysh I've added --skip-init to commonFlags, the help docs, and made sure to add a comment to commonFlags so that is more clear in the future. I also tested this locally and it's now working as I would expect. No rush, but I think it's now ready for a review 👍

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

thanks @Gowiem

@aknysh aknysh added the patch A minor, backward compatible change label Sep 10, 2022
@aknysh aknysh merged commit 317ed16 into master Sep 10, 2022
@aknysh aknysh deleted the add-skip-init branch September 10, 2022 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch A minor, backward compatible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants