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

commands: terraform add #28874

Merged
merged 9 commits into from
Jun 17, 2021
Merged

commands: terraform add #28874

merged 9 commits into from
Jun 17, 2021

Conversation

mildwonkey
Copy link
Contributor

terraform add generates resource configuration templates which can be filled out and used to create resources.

The template is output in stdout unless the -out flag is used. By default, only required attributes are included and the values all set to null, with a comment indicating the expect value type (string, list of object, etc). The -optional flag tells add to include all optional attributes as well.

When the -from-state=ADDRESS flag is used, add will copy the values from that resource's state into the template. For this initial release, terraform does not copy any values that are sensitive according to the provider schema; a future release may add a mechanism to overrides this behavior. (-from-state currently ignores the -optional flag and prints all attributes; this may change depending on user feedback).

The most-likely workflow with -from-state is using terraform import to import a resource's state, and then terraform add -from-state=ADDRESS NEW_ADDRESS.

The generated configuration is not likely to be precisely what's needed, and I expect that users will still need to modify the generated configuration block to suit their needs. Users will need to manage dependencies, replace hard-coded values with expressions (for e.g. references to variables), and make other manual adjustments.

While I wouldn't call this a fully experimental feature, its stability is not guaranteed. It is likely that flags will change once we start getting user feedback.

Here are some example outputs, using my perennial favorite random_pet resource:

» terraform add random_pet.meow 
resource "random_pet" "meow" {
}

» terraform add -optional random_pet.meow
resource "random_pet" "meow" {
  keepers   = null
  length    = 3
  prefix    = "add"
  separator = "-"
}

 terraform add -from-state=random_pet.example random_pet.meow
resource "random_pet" "meow" {
  id        = "add-vigorously-refined-doe"
  keepers   = null
  length    = 3
  prefix    = "add"
  separator = "-"
}

terraform add ADDRESS generates a resource configuration template with all required (and optionally optional) attributes set to null. This can optionally also pre-populate nonsesitive attributes with values from an existing resource of the same type in state (sensitive vals will be populated with null and a comment indicating sensitivity)
@mildwonkey mildwonkey requested a review from a team June 3, 2021 13:31
Optional bool

// Provider specifies the provider for the target.
Provider addrs.Provider
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm kinda curious about the use-cases for this argument. I'm fine with including it if we can imagine situations where it would be important, but I'm asking because I believe we only had this argument on some other commands like terraform import because historically they didn't refer to configuration at all, and only later did we make them consult the configuration to see which provider to use; given that we're not constrained by backward compatibility on this new command, I'd lean towards leaving this out if we can, to have fewer usage variations to support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use-case is the (unlikely!) scenario when you have multiple providers with the same type in a configuration. If I had a configuration with both hashicorp/random and mildwonkey/random, I'd need to tell terraform which provider I meant if it weren't the default (either an implied default, or the provider with a local name of "random").

This is important because (I know you know this, but writing out for completeness) those providers could have entirely different schemas and resource types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now that I say this, the google-beta provider is another example of a more-plausible real world use case!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! In retrospect I think I had my wires crossed when I originally wrote this comment, because I was still thinking about terraform import (adding things to the state) rather than terraform add (adding things to the config). But with fresh eyes this makes perfect sense.

This does lead me to a different question, though: in order to later fold the import functionality into this, we'll need to know a specific provider configuration to use, rather than just a provider. We'd need it both to know which provider configuration to run the import operation with, and also to know what to write into the provider argument in the generated configuration to make sure it ends up associated with the same provider configuration that imported it.

Given that, I wonder if we should pre-emptively set up this argument as an addrs.AbsProviderConfig instead of just an addrs.Provider, so that we'll be ready for the later extension. The downside is that the syntax for those is a bit clunky to write on the command line: -provider='module.bar.module.baz.provider["registry.terraform.io/hashicorp/aws"].foo' ... but hopefully this option is rarely used enough that it being a bit clunky is okay in return for it being explicit.

Perhaps even in the short term we could use the more elaborate form of this to fill in the provider argument in the generated resource block, so that for my example above it would write provider = aws.foo. (In the -from-state case we can infer a full provider configuration address from the state, so I think for now it'd only be for the mode where it's only generating a placeholder skeleton.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 That's is a very good point, thank you.

add does already fill in the provider argument when the given provider doesn't match the implied provider, but - as you have just pointed out - it would be missing any alias. I'll make that change!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've refactored with this change and updated the tests.
One notable change is I've now made -provider and -from-state conflicting flags; if -from-state is selected we will get the provider directly from state (looking up the provider local name from configuration, as before, but now we know we're using the correct provider FQN).

@mildwonkey mildwonkey requested a review from a team June 7, 2021 17:19
There are two main benefits to this change:
- add can now properly write the provider attribute in the generated resource config; the first iteration did not take any aliases into account
- when we later extend add to include import functionality, we will know what specific provider configuration to use (if it's not the implied default)
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

Sorry for the late-arriving review, here. Got a bit distracted by release chores! 😬

This looks great to me! It seems like a nice, independently-useful feature which also gives us space to grow additional use-cases related to importing later.

I left some comments inline that are the usual subjective opinionating and nit-picking which, as always, you should feel free to ignore or adjust based on your own judgement.

helpText := `
Usage: terraform [global options] add [options] ADDRESS

Generates a blank resource template. With no additional flags,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a total nit 😬 but since we're saying "options" everywhere else here (which is consistent with how we've been evolving our docs/usage output elsewhere), it'd be nice to be consistent here and say "additional options" instead of "additional flags".

``,
},
"-from-state": {
[]string{"-from-state=test_foo.bar", "module.foo.test_foo.baz"},
Copy link
Contributor

Choose a reason for hiding this comment

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

This scenario of using one resource instance to generate the config of another feels a little weird to me... I can imagine situations where it might make sense, like if it's a super-general resource that doesn't contain any unique identifiers that would conflict, but I guess my instinct here is to optimize for the common case of generating a configuration for the same address that we're reading from in the state, which would simplify this command line and remove an opportunity for a typo-related mistake:

terraform add -from-state module.foo.test_foo.baz

In this structure, the "from address" would be forced to be module.foo.test_foo.baz, matching the address of what we're generating.

I'm thinking here about the similar situation we had with terraform import historically, where you could import into something that wasn't in the configuration so if you made a typo then Terraform would plan to destroy the thing you just imported and create a new thing with the correct name. Allowing a different source address than the generated configuration seems like an opportunity for the same outcome from the opposite direction, which I'd rather avoid if we can. (Presumably the user can always change the name label in what Terraform generates here, if they really do want a different name, right?)

What do you think? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had identified this use-case all along (mostly for newer terraform adopters), and it's what I'd set out in the RFC, so I feel a little hesitant about changing it now. Someone could also conceivably want to use it to generate configuration for an entirely different workspace (not in the terraform workspace sense; an unrelated set of configuration files).

Having said that, I'm speaking from my personal biases as a former terraform configuration author. I would have used add this way, so I assume that others will too. 🤷

I'm willing to make this change, but I'm not entirely convinced - we can chat about this outside of GH.

After some internal back and forth, we've settled on `-from-state` as a bool, which limits users to generating configuration from the same address as the target address (which is to say: generate config for something that's already been imported). We decided that it would be easier to add something like a "-force" flag to allow generating config from a different resource than it would be to change the flag from a string to a bool after the fact.
@mildwonkey mildwonkey merged commit 583859e into main Jun 17, 2021
@mildwonkey mildwonkey deleted the mildwonkey/add branch June 17, 2021 16:08
@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants