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

CLI Redesign #3445

Open
yhakbar opened this issue Sep 27, 2024 · 10 comments
Open

CLI Redesign #3445

yhakbar opened this issue Sep 27, 2024 · 10 comments
Labels
accepted Accepted RFC rfc Request For Comments

Comments

@yhakbar
Copy link
Collaborator

yhakbar commented Sep 27, 2024

Summary

This RFC was initially vetted in a Gruntwork internal Notion page, and increased in size and complexity until it became detailed enough that it was no longer practical to publish as a GitHub issue.

To read the RFC, navigate to the publicly published version of the Notion page here:
https://gruntwork.notion.site/PUBLIC-Terragrunt-CLI-Redesign-10ed8759a6828042ac6bc03aef4d9dfb

That page will be updated according to feedback, with comments placed on this issue to track those updates.

All feedback, conversation and updates will take place on this issue.

@yhakbar yhakbar added rfc Request For Comments pending-decision Pending decision from maintainers labels Sep 27, 2024
@slaughtr
Copy link

slaughtr commented Oct 1, 2024

It's great to see this RFC, this would surely clean up some mess and make Terragrunt more clear to new users. That said, it's not super clear in the text how the new run and exec commands would work. I'd love to see more examples comparing common use cases in new vs old style.

For my two cents on specific changes:

  • The -- syntax is familiar to me, and probably many people. It's the same syntax that many other tools use (aws-vault and chamber being two common ones). The only issues I could see with it are how it might interact with those other tools, but as I recall from combining aws-vault and chamber in the past that might not be a problem after all?
  • Removing terragrunt from CLI flags makes sense, though per the RFC flag collisions would be a concern. I'd also wonder whether removing terragrunt from the flags would really help with anything more than a reduction in characters - right now it's very obvious to users when a flag relates only to Terragrunt, and this change would remove that clarity. If I were in your shoes and erring on the side of caution, I'd probably not make this change/make this change much later in the rollout after people have had some time to get familiar with the rest of the changes.
  • The approach to backend provisioning absolutely will reduce confusion for many people. Teams I support run into confusion with the current approach very often. 10/10

@yhakbar
Copy link
Collaborator Author

yhakbar commented Oct 2, 2024

Thanks for the feedback @slaughtr !

run vs exec

I'll think through that feedback and add more detail to distinguish run vs exec in the RFC. It's feedback that I also got internally, and I want to address it well.

The short explanation of the difference between them is that exec will spawn a process with any command and arguments you want, while run will always spawn a process that uses tofu or terraform. The reason this difference is useful enough to warrant a dedicated command is that Terragrunt inspects the tofu/terraform command being run to perform some logic that makes life easier for users, it doesn't just run them blindly.

e.g. Any command that would fail without tofu/terraform init will automatically run init first if you don't opt-out.

Many users leverage Terragrunt more than to simply wrap tofu/terraform, however.

e.g. Many users take advantage of terragrunt-iam-role to assume an AWS IAM role prior to executing a tofu/terraform command via terragrunt.

This is really convenient, but sometimes you want that convenience without also running tofu/terraform.

e.g. Maybe you want to list all the S3 buckets in an account that you would assume a role into using Terragrunt without running tofu/terraform, but still have Terragrunt assume that role for you automatically using the automatic role assumption.

Right now, you could add a run_cmd or a hook to do that while running an unrelated command like terragrunt init, but it's an indirect way of getting to some useful value out of Terragrunt. A more convenient mechanism would be to directly expose the ability to run an arbitrary command.

The -- Argument

We would have to implement it and test it to be sure, but typically, the way that CLI libraries handle the -- argument is to say "Hey CLI parser, I know that the arguments after this string might look like flags because they can start with -. I want to be explicit with you that they aren't. Ignore the --, and treat every string after this as an argument. I would expect every tool that respects -- would be pretty easily handled.

Removing terragrunt From Flags

Noted. I'll add that as a consideration for the migration plan. Other work can definitely be prioritized ahead of it.

Other things that can be improved by doing this are things like the following:

  • Provides an opportunity to revisit names of existing flags so they make more sense and are grouped together better.

    e.g.

    terragrunt-iam-role --> iam-assume-role

  • Provides an opportunity to make Terragrunt flags more useful.

    e.g.

    terragrunt-no-color specifically turns off color in Terragrunt terminal output. Right now, users have to run terragrunt plan --terragrunt-no-color -no-color in order to have color disabled in both Terragrunt and OpenTofu/Terraform. I would be confusing if terragrunt-no-color did both, as the flag specifically says that it's controlling Terragrunt behavior. Having a no-color flag that turned off color for both Terragrunt and OpenTofu/Terraform, and a dedicated flag added later called something like no-tg-color would make it so that users get the best default experience using the simplest flag.

  • Provides an opportunity to confuse new users less.

    e.g.

    terragrunt --terragrunt-log-level debug terragrunt-info can look really confusing to new users.

    Why is it saying terragrunt so much?... Isn't that obvious, given that it's the name of the tool?

    Compare that to:

    terragrunt --log-level debug info print

I don't think it's a small thing that the number of characters in flags would be significantly reduced, btw. Some people don't type that fast, and longer flags are harder to read quickly, as users end up having to break flags over multiple lines, etc to read them conveniently.

@josh-padnick
Copy link
Contributor

josh-padnick commented Oct 3, 2024

It's a small comment, but if we decide it's worth continuing to namespace our flags, maybe using --tg-flag-name makes life a lot better.

@josh-padnick
Copy link
Contributor

One other idea: If users find the difference between run and exec confusing, perhaps we should stick with just one of them (let's say run) but detect when we call a terraform or tofu command and then act accordingly?

For example, terragrunt run -- tofu plan would be the same as terragrunt exec plan as described in the RFC, but now we have one command to handle.

We could even allow running tofu or terraform without "magic" by adding support for another flag. Something like: terragrunt run --no-tf-magic -- tofu plan (though hopefully with a more thoughtful param name).

@yhakbar
Copy link
Collaborator Author

yhakbar commented Oct 3, 2024

@slaughtr + @josh-padnick,

I've attempted to address both your concerns with these two updates to the RFC, let me know if you have any more feedback!

Personally, I don't think Terragrunt should be namespacing its own flags at all. No other tool I know of does this, and it makes the experience of using Terragrunt worse. The proposal provides tools for disambiguating flags like --, so why not leverage that instead?

@slaughtr
Copy link

slaughtr commented Oct 4, 2024

Y'know, with those extra examples and your points above I now agree. :shipit:

@josh-padnick
Copy link
Contributor

josh-padnick commented Oct 7, 2024

+1 to @slaughtr's comment!

@gmaghera
Copy link

gmaghera commented Oct 24, 2024

I like these changes. Terragrunt flags have been difficult to remember, for me, because of the namespacing.

One thing that has been on my wishlist for terragrunt, which may be outside of scope here, is the ability to use the console subcommand to inspect Terragrunt variables. In the current form, the command takes place in the Terraform/Tofu layer, where I cannot directly look at variables set or calculated in the Terragrunt layer.

@yhakbar
Copy link
Collaborator Author

yhakbar commented Nov 19, 2024

An update on this RFC:

Due to the confusion in issue #3533, the RFC has been updated to change the flag name --debug to --debug-inputs, which more accurately reflects what it does.

It seems like the community is largely OK with the changes in this RFC otherwise.

For transparency, I'm intentionally leaving this RFC in pending-decision, as the RFC #3134 is still in development. That one is almost complete, however, and once it is, I will mark this RFC as accepted and start scheduling work to deliver the changes. We're trying to avoid having too many active RFCs at once so that it's clear to the community what major changes are taking place in the product, and so that there isn't conflict between delivery of different feature sets.

If you have any major objections to this RFC make them known soon!

@yhakbar
Copy link
Collaborator Author

yhakbar commented Dec 4, 2024

With the conclusion of #3134 , this RFC is now ready to be marked accepted, and for work to be scheduled.

We'll look to start releasing progress on in it in the near future (within the next two weeks).

@yhakbar yhakbar added accepted Accepted RFC and removed pending-decision Pending decision from maintainers labels Dec 4, 2024
@yhakbar yhakbar mentioned this issue Dec 21, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Accepted RFC rfc Request For Comments
Projects
None yet
Development

No branches or pull requests

4 participants