Environments #12182

jbardin · 2017-02-22T23:19:59Z

This starts support for "environments" in the backends.

Environments, aka "named states", are optionally supported by Backend implementations. Because they are optional, the corresponding method set has been split out into its own interface:

// MultiState is an interface that a backend can implement to allow changing
// between named states depending on the configured environment.
type MultiState interface {
	// States returns a list of configured named states and the current state.
	States() ([]string, string, error)

	// ChangeState changes to the named state. If the named state doesn't exist
	// it will be created.
	ChangeState(name string) error

	// DeleteState removes the named state if it exists. If the current state is
	// deleted, the backend should change to the default state. It is an error
	// to delete the default state.
	DeleteState(name string) error
}

This diverges slightly from the RFC in a few ways.

There's no name argument added to the State method. This would change a lot of code, and since the backend must already know about the current state, it is mostly superfluous. The primary drawback is that it requires a call to ChangeState to fetch a particular state.
States() returns the list of known states, and the current state. The alleviates the state.State from having to know about its name.
Added DeleteState.

An "environment" corresponds to a named state. Environments can be manipulated through the new terraform env command. This is implemented as a single command with flags for convenience, but could be refactored into subcommands if it improves the UI.

Usage: terraform env [options] [NAME]

  Create, change and delete Terraform environments.

  By default env will list all configured environments. If NAME is provided,
  env will change into to that named environment.


Options:

  -new=name      Create a new environment.
  -delete=name   Delete an existing environment,

  -state=path    Used with -new to copy a state file into the new environment.
  -force         Used with -delete to remove a non-empty environment.

jbardin · 2017-02-23T15:01:17Z

@mitchellh,

After discussing with @apparentlymart, we agree that subcommands are a cleaner solution than command flags and flag-specific flags. The awkwardness seems to be around the bare env command taking an argument or a subcommand, which makes it less intuitive and has the unfortunate possibility of subcommand and environment name collision (without also supporting the non-obvious -- end of options delimiter).

How about the following command set?

- env: print usage
   |- list: list environments
   |- new: create new env, and change to that env
   |- select: change to the named env
   |- delete: remove the selected env

I'm not opposed to list being the default command for env, but I do like the immediacy of printing the usage for new users, rather than requiring the -h option.

apparentlymart · 2017-02-23T15:40:52Z

This subcommand-based interface makes good sense to me. Perhaps a reasonable compromise on the bare terraform env command would be for it to produce both status information and usage, to help explain the context:

Current Environment is production

Subcommands:
  list: list all environments
  select <name>: select the given environment for subsequent operations
  new <name>: create a new environment and select it
  delete <name>: delete the given environment

This way the output tells us where we are and where we can go from here. I presume here that running e.g. terraform env new or terraform env new -h would provide a more detailed version of the usage of that command, so we don't need to overwhelm the user with all the flag details in the initial output.

Some of the subcommand flags could be progressively discovered through good error messages. For example:

$ terraform env delete production
The environment "production" contains resources.
Deleting it will cause Terraform to lose track of these resources.

Either select this environment and destroy the resources first, or use the -force
option to delete Terraform's record of these resources without destroying them
first.

After using -force it will be necessary to use "terraform import" to bring
these resources back under Terraform's management.

The -state one isn't really a response to an error, so there's not such a good path to discovering that one, but presumably importing an existing local state as an environment is a rare enough situation that it's fine to make that one be discoverable only by explicitly looking in the docs/usage, just like the -state options on many other Terraform commands.

jbardin · 2017-02-23T19:26:54Z

Pushed with the new subcommands, but I'm retracting the current env output from the bare env command. Because a config directory could be provided as an argument, there could again be a conflict with subcommands and paths. It's not impossible to add, but I'm leaning towards simplicity for now.

jonmorehouse · 2017-02-24T02:44:48Z

@jbardin this is a really interesting concept; you mentioned an RFC in your original description. Is that something shareable here? I'd personally love to read a bit more about the backstory of this and intended use cases etc.

Thanks!

jbardin · 2017-02-24T15:47:20Z

@jonmorehouse,

Sorry, I don't have anything I can share, but at this point this PR probably contains more relevant info than the original RFC ;).

The immediate use case for most developers is replacing the workflow of having multiple state files manually managed and specified at runtime, to a workflow of using environments to manage those state files in a less error-prone way.

I think the only missing concept here is that "Enhanced Backends" will have the ability to extend the idea of an "environment" further, as they handle the Terraform operations directly. What this means precisely will depend on the backend implementation.

mitchellh

The CLI looks super good, very little to no comments there. Some seemingly major but probably not so bad feedback on the main implementation. I think overall its probably mostly just bit shuffling a bit but important for the semantics.

We can chat in slack this week too! Almost there, looks really good.

mitchellh · 2017-02-27T05:50:43Z

backend/local/backend.go

+
+	// workingDir is where the State* paths should be relative to.
+	// This is currently only used for tests.
+	workingDir string


I'd avoid this and use testChdir. We actually USED to have a field like this but I've tried to make helpers to make sure we're executing code identical as much as possible (we're not perfect yet).

Yeah, I agree. This was solely to check be able to differentiate 2 backends in a test, where I should have made a test implementation instead.

mitchellh · 2017-02-27T05:54:55Z

backend/backend.go

@@ -27,6 +29,22 @@ type Backend interface {
 	State() (state.State, error)
 }

+// MultiState is an interface that a backend can implement to allow changing
+// between named states depending on the configured environment.
+type MultiState interface {


I'd prefer to make this required for all states (make it part of Backend).

For legacy remote state we can return an error in the actual remote-state.Backend implementation instead.

The reason being is that going forward I don't want this to be optional and introduce more optional interfaces. It is difficult now due to backwards compatibility but if we force all new remote implementations to support it and only error for legacy then we can slowly fix them over one by one as we convert to the new API.

I believe that should be fully backwards compatible and simplifies all the checking.

I like to err on the side of smaller interfaces (isn't there some tao of Go quote about small interfaces? 😉 ) Though if we do want to enforce this it's not too unwieldy and not nearly the largest in terraform.

It may be, but I also think an interface should represent the full set of functionality required to make it useful. In the context of Terraform, I believe that is loading AND changing states, so splitting it into two required interface seems more of a mental burden than having one required interface.

So I think it should be part of the one interface.

Yup, advocating the alternative if only to keep ourselves in line ;)

mitchellh · 2017-02-27T05:55:57Z

backend/backend.go

+// between named states depending on the configured environment.
+type MultiState interface {
+	// States returns a list of configured named states and the current state.
+	States() ([]string, string, error)


The backend shouldn't be responsible for "current" state since that should be local to the user (i.e. with the same remote backend on two different machines one user can point to one env and another can point to another. The backend itself has no concept of "current").

The "current" state should be stored locally in the terraform cache dir (.terraform so it is VCS ignored).

Ok, this makes sense, and I clears up part of what I wanted to cover in this first review. The original spec called for a remote ChangeState method, which implied the backend also managed the current state. Deferring to the backend was much less intrusive in the terraform code, not even accounting for possible synchronization issues between the local environment and backend. Removing the backend from being aware of "current" at all makes this work much better.

Ah I can see how that is confusing. Sorry!

Given that API, maybe the behavior should be to remove ChangeState and just have State(name), DeleteState(name), States() []string error?

Yes, that's what I was about to implement. I can't think of a reason to require a separate CreateState(string) method, when State could handle it.

Agreed, and state existence when switching to it can live in the CLI itself. I think thats still important to avoid a typo when switching states.

mitchellh · 2017-02-27T05:57:08Z

backend/local/backend.go

+	// the listing always start with "default"
+	envs := []string{backend.DefaultStateName}
+
+	current, err := b.currentStateName()


Same as above, "current" state should be a TF core thing and backends should just be told directly what state to load. This will simplify backend implementation a lot and shift that complexity into core but at least its shared.

mitchellh · 2017-02-27T05:57:53Z

backend/local/backend.go

+	}
+
+	// if we're deleting the current state, we change back to the default
+	if name == current {


Deleting the current state isn't allowed (as per the spec, we have to show an error and tell the user to switch to another state first). This protects the user by making it so they 1.) must have another state to go to and 2.) can't easily delete states.

Oh yeas, I missed that and figured it OK since state was protected if it's not empty. Easy change.

mitchellh · 2017-02-27T05:59:32Z

backend/remote-state/backend.go

@@ -46,6 +47,14 @@ func (b *Backend) Configure(rc *terraform.ResourceConfig) error {
 	return b.Backend.Configure(rc)
 }

+func (b *Backend) States() ([]string, string, error) {
+	return []string{backend.DefaultStateName}, backend.DefaultStateName, nil


As per the comments above: this is where I'd move the "multiple states not supported" errors.

Add the missing methods/arguments to handle Terraform environments in Backends. Extra functionality simply returns defaults for now.

Add the functionality required for terraform environments

The Local backend is now responsible for handling the paths to the local state files, since they are dependent on the current environment.

Split the interface to change environments out from the minimal Backend interface, to make it optional for backend implementations. If backend.MultiState isn't implemented, return a "not implemented" from environment related methods. Have the Local backend delegate the MultiState methods to the proper backend.

Ensure that when MultiState methods are properly delegated when there is a defined Local.Backend.

Used a single command with flags for now. We may refactor this out to subcommands.

In order to operate in parity with other commands, the env command should take a path argument to locate the configuration. This however introduces the issue of a possible name conflict between a path and subcommand, or printing an incorrect current environment for the bare `env` command. In favor of simplicity this removes the current env output and only prints usage when no subcommand is provided.

Forgot to remove the currentState field, which was not always set. The current state should always just be read from the environment file. Always return the default state name when we can't determine the state.

Destroying a terraform state can't always create an empty state, as outputs and the root module may remain. Use HasResources to warn about deleting an environment with resources.

These were left from the initial implementation, but are not used.

What will hopfully be the final version of the Backend interface. This combines the MultiState interface into Backend since it will be required to implement, and simplifies the interface because the Backend is no longer responsible for tracking the current state.

Update the methods, remove the handling of "current", and make tests pass.

move the unsupported error value to backend.ErrNamedStatesNotSupported to be used by any backend implementation.

Add Env and SetEnv methods to command.Meta to retrieve the current environment name inside any command. Make sure all calls to Backend.State contain an environment name, and make the package compile against the update backend package.

Add an `environment` field to the terraform remote state data source.

add a test to ensure we have consistent output

mitchellh

One nitpick.

Looks great! I think we got it. :)

One question: why did we have to change States to embed StateMeta vs. Meta?

mitchellh · 2017-03-01T00:29:10Z

command/env_delete.go

+
+	// Lock the state if we can
+	lockInfo := state.NewLockInfo()
+	lockInfo.Operation = "env new"


"env delete" probably

mitchellh · 2017-03-01T00:31:30Z

command/state_meta.go

@@ -23,8 +25,9 @@ func (c *StateMeta) State(m *Meta) (state.State, error) {
 		return nil, err
 	}

+	env := c.Env()


May want to be using m.Env() here. Practically I don't think it makes a difference currently but maybe one day we may pass in a differently configured meta here?

Ah, ok. This was the reason for your question above, in that StateMeta was calling the embedded Meta.Env method, and embedding Meta in StateMeta was just the cleanest way to ensure all State commands also had the same method.

Looking at this though, I think we can also just drop the Meta param, since StateMeta would now have the correctly configured Meta for the command embedded.

Oh, actually there is no more Meta.State with the new backends, so this structure probably isn't needed at all.

I finally see what the second Meta is for now. I'm going to split the Meta back out just to alleviate the confusion, since StateMeta doesn't it need.

Removing the call to StateMeta.Env, so that it doesn't need an embedded Meta field. Embed Meta and StateMeta separately in all State commands.

jbardin · 2017-03-01T15:29:51Z

Fixed lock reason for env delete and split Meta back out of StateMeta since it turns out StateMeta doesn't need the Env method itself.

ghost · 2020-04-16T02:05:38Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

jbardin added core enhancement labels Feb 22, 2017

jbardin force-pushed the jbardin/environments branch 2 times, most recently from 4986ce6 to 5b6df06 Compare February 23, 2017 14:48

jbardin force-pushed the jbardin/environments branch 2 times, most recently from 6dfc6b9 to 4714299 Compare February 23, 2017 18:35

jbardin changed the title ~~[WIP] Environments~~ Environments Feb 23, 2017

jbardin requested a review from mitchellh February 23, 2017 19:21

mitchellh suggested changes Feb 27, 2017

View reviewed changes

jbardin added 13 commits February 28, 2017 16:03

Update Backend to incorporate environments

761c63d

Add the missing methods/arguments to handle Terraform environments in Backends. Extra functionality simply returns defaults for now.

Make the Local backend handle its own named states

dbc45b9

Add the functionality required for terraform environments

Remove state path handling from commands

1ea9413

The Local backend is now responsible for handling the paths to the local state files, since they are dependent on the current environment.

Add tests to check Backend delegation

e6eb71d

Ensure that when MultiState methods are properly delegated when there is a defined Local.Backend.

Add basic env commands

31f0338

Used a single command with flags for now. We may refactor this out to subcommands.

split the env command into subcommands

c852648

fix incorrect current state in local backend

fbc11c7

Forgot to remove the currentState field, which was not always set. The current state should always just be read from the environment file. Always return the default state name when we can't determine the state.

use State.HasResources rather than State.Empty

2a73331

Destroying a terraform state can't always create an empty state, as outputs and the root module may remain. Use HasResources to warn about deleting an environment with resources.

remove some leftover methods in the legacy backend

7f453f3

These were left from the initial implementation, but are not used.

update local.Local to match the latest Backend

65527f3

Update the methods, remove the handling of "current", and make tests pass.

jbardin force-pushed the jbardin/environments branch from f97866d to c30eee1 Compare February 28, 2017 21:34

jbardin added 2 commits February 28, 2017 16:35

Make backcend/legacy match new Backend iface

5762878

move the unsupported error value to backend.ErrNamedStatesNotSupported to be used by any backend implementation.

update remote-state.Backend

8fdf3a4

jbardin added 5 commits February 28, 2017 16:35

update remote-state/inmem client

597eb6c

update remote-state/consul

f866bb5

Thread the environment through all commands

b53704e

Add Env and SetEnv methods to command.Meta to retrieve the current environment name inside any command. Make sure all calls to Backend.State contain an environment name, and make the package compile against the update backend package.

update the remote state datasource

6c3800d

Add an `environment` field to the terraform remote state data source.

fix rebased tests

dc67554

jbardin force-pushed the jbardin/environments branch from c30eee1 to dc67554 Compare February 28, 2017 21:35

Local.StatePaths doesn't need to reutrn an error

4dac986

add a test to ensure we have consistent output

mitchellh approved these changes Mar 1, 2017

View reviewed changes

Split Meta back out of StateMeta

39a5ddd

Removing the call to StateMeta.Env, so that it doesn't need an embedded Meta field. Embed Meta and StateMeta separately in all State commands.

jbardin force-pushed the jbardin/environments branch from d8c9523 to 39a5ddd Compare March 1, 2017 15:20

jbardin merged commit f9aa3d3 into master Mar 1, 2017

jbardin deleted the jbardin/environments branch March 1, 2017 15:30

nanoz mentioned this pull request Mar 1, 2017

config: validate backend configuration can't contain interpolations #12067

Merged

ghost locked and limited conversation to collaborators Apr 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Environments #12182

Environments #12182

jbardin commented Feb 22, 2017 •

edited

Loading

jbardin commented Feb 23, 2017 •

edited

Loading

apparentlymart commented Feb 23, 2017

jbardin commented Feb 23, 2017

jonmorehouse commented Feb 24, 2017 •

edited

Loading

jbardin commented Feb 24, 2017

mitchellh left a comment

mitchellh Feb 27, 2017

jbardin Feb 27, 2017

mitchellh Feb 27, 2017

jbardin Feb 27, 2017

mitchellh Feb 27, 2017

jbardin Feb 27, 2017

mitchellh Feb 27, 2017

jbardin Feb 27, 2017

mitchellh Feb 27, 2017

jbardin Feb 27, 2017

mitchellh Feb 27, 2017

mitchellh Feb 27, 2017

mitchellh Feb 27, 2017

jbardin Feb 27, 2017

mitchellh Feb 27, 2017

mitchellh left a comment

mitchellh Mar 1, 2017

mitchellh Mar 1, 2017

jbardin Mar 1, 2017

jbardin Mar 1, 2017

jbardin Mar 1, 2017

jbardin commented Mar 1, 2017

ghost commented Apr 16, 2020

Environments #12182

Environments #12182

Conversation

jbardin commented Feb 22, 2017 • edited Loading

jbardin commented Feb 23, 2017 • edited Loading

apparentlymart commented Feb 23, 2017

jbardin commented Feb 23, 2017

jonmorehouse commented Feb 24, 2017 • edited Loading

jbardin commented Feb 24, 2017

mitchellh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchellh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbardin commented Mar 1, 2017

ghost commented Apr 16, 2020

jbardin commented Feb 22, 2017 •

edited

Loading

jbardin commented Feb 23, 2017 •

edited

Loading

jonmorehouse commented Feb 24, 2017 •

edited

Loading