command: adjust exit code of state rm

[jgpacker]

Command state rm will now exit with code 1 when the resource is not found in the current state.
This is consistent with terraform state show non_existent_resource.

Fixes #17800

[pselle]

Hi @jgpacker, thanks for this contribution! I tried out your patch and it works as described, however, now that exit code is 1, we should show an error to the user versus showing UI output -- so that the experience is consistent (the command errored, so an error is displayed).

In Terraform, our best practice to add an error is using the tfdiags package, where a summary, and then more helpful information is shared so the user can amend their error. In this scenario, something like (this uses some language from the error displayed when you use terraform show on the non-existent resource) could replace the current UI output in state rm:

		diags = diags.Append(tfdiags.Sourceless(
			tfdiags.Error,
			"Invalid target address",
			"No matching objects found. To view the available instances, use "terraform state list". Please modify the address to reference a specific instance.",
		))
		c.showDiagnostics(diags)
		return 1

[jgpacker]

Thank you for the feedback. I will correct this soon

[jgpacker]

I have updated the code according to the feedback. Unfortunately I have not been able to test it locally right now. So there might be still be some adjustments to be done. Thank you in advance for the review

[pselle]

One improvement for state_mv, since we have a constant that already exists.

I had trouble figuring out a flow of terraform state mv that would hit this while testing on my machine -- perhaps you could add a test to state_mv_test.go hitting this error?

Outdated, changes made

[jgpacker]

@pselle I opened this PR only to fix state rm, but I wanted to make sure other state subcommands were consistent with this behavior. I changed state mv because there is an if related to this condition, but I'm not sure how to reproduce it in this case.
I would not be surprised if this condition is not reached at this moment.
Should we remove this condition? Make the error message more generic?
What do you suggest?

[pselle]

Ah in that case (if you haven't been able to replicate through with state mv and the goal was state rm with mv being a "while I'm here...") I would suggest we only fix state rm and leave mv be. That follows the line of keeping a change focused and tested, so while mv might have some tricky bits in it, if changing it is not our goal, we should leave it unchanged and leave that for future work :)

[jgpacker]

Thank you for the orientation. I have reverted the changes in state_mv. Please review again

[pselle]

Thanks for the nudge @jgpacker. I'm approving this, but tagging it with "breaking-change" as the related issue has the same tag, so this will need to go out in the 0.13 release vs a 0.12.x release.

[ghost]

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.