Make Taint command use Resource Address #12289

huydinhle · 2017-02-27T21:37:52Z

Issue

UX bug: when using count, the plan/apply command and the taint command require different ways to refer to a specific resource instance #11045

Doc Update

I don't think we need doc update on the website, but we can clarify it if we really need to
I think the release note should be enough for this issue

Relevant Terraform version

This is not urgent to be deployed immediately
It can wait for the next release of terraform

About Acceptance Test.

I'll be running acceptance test in aws
I'm still working on writing the acceptance test for this code

Questions

Do I need to run acceptance test on Azure and Google also?
I ran the acceptance on aws and found no problem, should I save the log and report it here?
@heraclmene @apparentlymart

josephholsten · 2017-06-15T09:04:18Z

@apparentlymart seems like this is a breaking change, but wanted: #11045 (comment)

apparentlymart · 2017-06-15T17:18:23Z

Indeed this seems like a good change for consistency, and I think the breaking change shouldn't be too bad since the resource address syntax is pretty similar to what we previously expected, some details notwithstanding.

We don't actually expect acceptance tests for this sort of core functionality, in the same sense as we would for providers, but rather they are usually tested via a mixture of command-line-level tests in the command package and internal-ish tests in the terraform/context_*_test.go tests.

I think this change is definitely a good idea though I think it might be a little close to 0.10 to add it to the scope of that release now. I'll pencil it in to the list and we'll see if we have time to look at it in more detail before the release. If not, we'll look at it for a later release.

Thanks for working on this, @huydinhle!

huydinhle · 2017-06-19T22:40:22Z

@apparentlymart no problem at all!

apparentlymart

Hi @huydinhle! Thanks again for the great work here and sorry I let it sit here for so long.

I've left some feedback inline. I understand that I've kept you waiting for a while here, so if you're unable to respond to this feedback there's no worries at all; I can plan to work on these remaining asks to get this ready if you no longer have time.

apparentlymart · 2017-06-28T00:19:27Z

command/taint.go

 	} else {
-		module = "root." + module
+		resourceAddressStr = "module.root.module." + module + "." + name


For resource addresses we actually don't expect any prefix for the root module. That is, a resource aws_instance.baz in the root module just has the resource address aws_instance.baz rather than module.root.aws_instance.baz. The module.blah. prefix is only used to traverse through child modules.

I think we could actually drop support for the -module flag here, given that we're considering this a breaking change anyway. Tainting a resource in a child module can be expressed as terraform taint module.foo.aws_instance.baz once we're using the standard resource address syntax.

@apparentlymart Can you help me on this since I haven't worked on this for a while? If I don't put use module.root.aws_instance.bazas resource address then when I call terraform.ParseResourceAddress(resourceAddressStr) always give me rsa.Path = [] in turns make the state.ModuleByPath retun nil ---> making taint can't find resource state

apparentlymart · 2017-06-28T00:20:10Z

command/taint.go

-			return c.allowMissingExit(name, module)
-		}
+	// Get resource type from name
+	resourceType := strings.Split(name, ".")[0]


If I'm understanding correctly, this information is available in rsa.Type, and so we shouldn't need to extract it and pass it around as a separate argument.

apparentlymart · 2017-06-28T00:23:11Z

terraform/state.go

+	// If there are no resources in this module, it is an error
+	if len(mod.Resources) == 0 {
+		return nil, fmt.Errorf(
+			"The module %s has no resources. There is nothing to taint.", module)


I assume this error message was from before you did some refactoring... with the current approach, it's not guaranteed that the caller here is retrieving the resource state in order to taint it.

Given that this function is general and in core, I'd suggest that rather than returning an error we instead return nil in all of these error cases and then produce suitable error messages in the UI code, where we have more context available about what operation is being performed.

huydinhle · 2017-06-29T18:57:08Z

@apparentlymart I would love to be able to finish the PR unless we are in a rush to have PR in. My bad for getting back to you late.

apparentlymart · 2017-06-30T17:56:40Z

Hi @huydinhle!

By all means you are welcome to finish this yourself! The only slight time pressure here is that the next 0.10 beta is likely to represent a feature freeze, so I'd ideally like to land this in the next week or so, to make sure there's enough runway to get it in there. However, I don't expect you to rush to meet our schedule! If all else fails, we can include it in a future release.

Thanks again!

huydinhle · 2017-07-09T20:42:04Z

@apparentlymart I think I addressed all the comments. Can you let me know if I need to do anything else. I would try to be actively working on this PR to get it merged. Thank you !

apparentlymart · 2018-02-02T20:47:05Z

Hi @huydinhle! Sorry for the long silence.

Unfortunately I didn't see your comment here until today 😖 and so I missed reviewing this for the 0.11 release too.

I've labelled this with breaking-change to remind us to look at it again for merging as part of the forthcoming 0.12 release. Since this changes some behavior of terraform taint we need to wait for a major release to give users a chance to update any automation they may have built around this command that depends on the old usage.

Sorry again for not noticing your updates and comment here before. This is a change I'm excited to merge since indeed the current difference in usage of this command is very confusing.

mochi99999 · 2018-03-01T00:40:36Z

Any updates on this? Just wanted to follow up as we recently ran into this issue on my team with the terraform taint resource addressing.

huydinhle · 2018-03-01T05:54:54Z

@apparentlymart it's no problem at all, I would love to continue to work on this PR to get it merged. Please let me know if anything else need to be done

huydinhle · 2018-03-01T05:55:47Z

@sonnyvan let's wait for the next steps from Terraform team 😄

Previously: Plan or Apply command UI is "terraform plan -target=aws_instance.kafka[0]" would work. However, that convention does not work with taint command because taint does not make use of resourceAdress. Example: $ terraform taint aws_instance.kafka[0] The resource aws_instance.kafka[0] couldn't be found in the module root These changes will make taint to follow the same convention with other commands. Example: ./terraform taint aws_instance.web[0] The resource aws_instance.web[0] in the module root has been marked as tainted!

huydinhle · 2018-03-02T21:46:09Z

@apparentlymart I haven't touched base with terraform in a while, can you give me some help to finish this PR ? thank you

pietermees · 2018-07-22T02:17:35Z

@apparentlymart is this making it into 0.12?

apparentlymart · 2018-07-23T16:48:25Z

Hi all! Sorry for the long silence here.

The work for v0.12 includes some significant changes to how state is modeled internally, and so we will need to change the implementation of the terraform taint command anyway in order to update it for the new internal models. When we make those changes we'll also adapt the work from this PR so that it'll use the standard resource address syntax, and merge all of these changes together.

apparentlymart · 2018-11-02T23:43:08Z

Hi again @huydinhle! Sorry for the long silence here.

The work for 0.12 in the end included writing a new parser for the resource address syntax that uses the same underlying mechanism as the configuration language itself. That means that resource addresses will always use the same syntax as references within the configuration.

Unfortunately as a result of this the taint command implementation changed a lot. While fixing that, we integrated the new parser and removed the -module option at the same time, which integrated the effect of your PR but written in a different way. This is why the changes in this PR are now conflicting with master.

I'm sorry that the changes here led to an independent implementation of what you implemented, rather than an adaptation of what you wrote as I originally suggested. I'm very grateful for the time you spent on this PR, and the discussions we had about it informed the design of the new implementation.

Since the functionality of this PR is now in master, I'm going to close this out. Thanks again for working on this, and I'm sorry again that the v0.12 work came into conflict with what you'd implemented here.

ghost · 2020-03-31T02:22:59Z

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.

huydinhle mentioned this pull request Feb 27, 2017

[WIP] Taint and resource address #12224

Closed

huydinhle changed the title ~~make use of resource address inside of taint command~~ Make Taint command use Resource Address Feb 27, 2017

huydinhle force-pushed the taint-resource-addresses branch 5 times, most recently from ada25d4 to 9b68a38 Compare March 1, 2017 18:59

paddycarver added bug core labels Mar 3, 2017

huydinhle force-pushed the taint-resource-addresses branch 2 times, most recently from 67641be to 1ddb236 Compare March 4, 2017 05:40

huydinhle force-pushed the taint-resource-addresses branch from 1ddb236 to 9a92108 Compare March 10, 2017 21:13

huydinhle force-pushed the taint-resource-addresses branch from 9a92108 to 7bd9724 Compare March 21, 2017 19:21

apparentlymart reviewed Jun 28, 2017

View reviewed changes

huydinhle force-pushed the taint-resource-addresses branch 5 times, most recently from c58d747 to a8f9c09 Compare July 9, 2017 20:35

huydinhle force-pushed the taint-resource-addresses branch from a8f9c09 to ef46064 Compare July 14, 2017 04:55

apparentlymart mentioned this pull request Aug 11, 2017

Terraform destroy destroys more resources than expected #15789

Closed

apparentlymart added the breaking-change label Feb 2, 2018

apparentlymart added the cli label Feb 2, 2018

apparentlymart removed the core label Feb 2, 2018

apparentlymart mentioned this pull request Feb 2, 2018

Terraform taint/untaint syntax inconsistent with 'plan', 'apply', 'state', etc. #11570

Closed

huydinhle force-pushed the taint-resource-addresses branch from ef46064 to 0d35c10 Compare March 1, 2018 06:08

huydinhle and others added 2 commits March 1, 2018 17:41

resourceAddressString to work with module test

6ab9db8

huydinhle force-pushed the taint-resource-addresses branch from 1b4ea04 to 6ab9db8 Compare March 2, 2018 01:42

This was referenced Aug 8, 2018

Add wildcard for tainting/untaining #18637

Closed

Replace multiple resource instances with a single -replace=... planning argument #1768

Open

apparentlymart closed this Nov 2, 2018

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Taint command use Resource Address #12289

Make Taint command use Resource Address #12289

huydinhle commented Feb 27, 2017 •

edited

Loading

josephholsten commented Jun 15, 2017

apparentlymart commented Jun 15, 2017

huydinhle commented Jun 19, 2017

apparentlymart left a comment

apparentlymart Jun 28, 2017

huydinhle Mar 2, 2018

apparentlymart Jun 28, 2017

apparentlymart Jun 28, 2017

huydinhle commented Jun 29, 2017 •

edited

Loading

apparentlymart commented Jun 30, 2017

huydinhle commented Jul 9, 2017

apparentlymart commented Feb 2, 2018

mochi99999 commented Mar 1, 2018

huydinhle commented Mar 1, 2018

huydinhle commented Mar 1, 2018

huydinhle commented Mar 2, 2018

pietermees commented Jul 22, 2018

apparentlymart commented Jul 23, 2018

apparentlymart commented Nov 2, 2018

ghost commented Mar 31, 2020

Make Taint command use Resource Address #12289

Make Taint command use Resource Address #12289

Conversation

huydinhle commented Feb 27, 2017 • edited Loading

Issue

Doc Update

Relevant Terraform version

About Acceptance Test.

Questions

josephholsten commented Jun 15, 2017

apparentlymart commented Jun 15, 2017

huydinhle commented Jun 19, 2017

apparentlymart left a comment

Choose a reason for hiding this comment

apparentlymart Jun 28, 2017

Choose a reason for hiding this comment

huydinhle Mar 2, 2018

Choose a reason for hiding this comment

apparentlymart Jun 28, 2017

Choose a reason for hiding this comment

apparentlymart Jun 28, 2017

Choose a reason for hiding this comment

huydinhle commented Jun 29, 2017 • edited Loading

apparentlymart commented Jun 30, 2017

huydinhle commented Jul 9, 2017

apparentlymart commented Feb 2, 2018

mochi99999 commented Mar 1, 2018

huydinhle commented Mar 1, 2018

huydinhle commented Mar 1, 2018

huydinhle commented Mar 2, 2018

pietermees commented Jul 22, 2018

apparentlymart commented Jul 23, 2018

apparentlymart commented Nov 2, 2018

ghost commented Mar 31, 2020

huydinhle commented Feb 27, 2017 •

edited

Loading

huydinhle commented Jun 29, 2017 •

edited

Loading