Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

json-output: Add change reasons to explain deletes #29649

Merged
merged 1 commit into from
Sep 24, 2021

Conversation

alisdair
Copy link
Contributor

The extra feedback information for why resource instance deletion is planned is now included in the streaming JSON UI output.

We also add an explicit case for no-op actions to switch statements in this package to ensure exhaustiveness, for future linting.

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.

Oh, interesting! I updated what is essentially the same thing over in the jsonplan package.

switch rc.ActionReason {
case plans.ResourceInstanceChangeNoReason:
r.ActionReason = "" // will be omitted in output
case plans.ResourceInstanceReplaceBecauseCannotUpdate:
r.ActionReason = "replace_because_cannot_update"
case plans.ResourceInstanceReplaceBecauseTainted:
r.ActionReason = "replace_because_tainted"
case plans.ResourceInstanceReplaceByRequest:
r.ActionReason = "replace_by_request"
case plans.ResourceInstanceDeleteBecauseNoResourceConfig:
r.ActionReason = "delete_because_no_resource_config"
case plans.ResourceInstanceDeleteBecauseWrongRepetition:
r.ActionReason = "delete_because_wrong_repetition"
case plans.ResourceInstanceDeleteBecauseCountIndex:
r.ActionReason = "delete_because_count_index"
case plans.ResourceInstanceDeleteBecauseEachKey:
r.ActionReason = "delete_because_each_key"
case plans.ResourceInstanceDeleteBecauseNoModule:
r.ActionReason = "delete_because_no_module"
default:
return nil, fmt.Errorf("resource %s has an unsupported action reason %s", r.Address, rc.ActionReason)
}

I didn't realize we had this in two places... updating them both to be in sync is a good immediate fix here, so let's indeed go ahead and merge this, but I think it'd also be good to think about how we can unify these two codepaths so that we only have to update one place and so that we don't risk them getting out of sync and using different strings to represent each case. 🤔

(Thankfully, it seems like you picked string values here that match what I added in the other place, so this'll be back to consistency for now.)

The extra feedback information for why resource instance deletion is
planned is now included in the streaming JSON UI output.

We also add an explicit case for no-op actions to switch statements in
this package to ensure exhaustiveness, for future linting.
@alisdair alisdair force-pushed the alisdair/json-ui-exhaustiveness branch from 633dfe2 to b699391 Compare September 24, 2021 17:17
@alisdair
Copy link
Contributor Author

Thankfully, it seems like you picked string values here that match what I added in the other place, so this'll be back to consistency for now.

Unfortunately for the previous set of action reasons I chose different values for the JSON string representations (not realizing that they were in the JSON plan, I guess?). So I think we are stuck with the divergence 😢

@alisdair alisdair merged commit a742d7e into main Sep 24, 2021
@alisdair alisdair deleted the alisdair/json-ui-exhaustiveness branch September 24, 2021 18:02
@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 Oct 25, 2021
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants