Incorporate missing post plan terminal statuses to ensure plan is completely processed before attempting to confirm run

[Uk1288]

Description

The changes in this PR checks potential terminal post plan statuses to ensure that a plan has been completely processed before proceeding to confirm the associated run. The terminal status changes depending on the post plan lifecycle that a run has. For example a plan can terminate at planned status or cost_estimated or policy_checked.

See issue for more details: #898

This PR depends on go-tfe PR#712

Testing plan

1. Workspace_run resource should create run as usual, the only way to produce the issue consistently is to force a run to remain in a status, e.g policy_checked, for a longer period of time such that workspace_run receives that status while polling the run. I achieved this by adding delays to each run state

External links

• Blocked by go-tfe PR

Output from acceptance tests

$ TESTARGS="-run TestAccTFEWorkspaceRun" make testacc

...

[brandonc]

Suggested change

	var policyOverridenStatuses = map[tfe.RunStatus]bool{
	var policyOverriddenStatuses = map[tfe.RunStatus]bool{

[brandonc]

In general, I find your diagnosis and fix to be convincing... but I'm also finding the helpers to be difficult to read and reason about and feel like we should take a moment to simplify them. I identified these specific things that could make this module simpler:

1. syntactically, I think there are two problems with awaitRun:
   a) nested switch statements-- I find it difficult to mentally retain a stack context once we reach that second nesting.
   b) this function takes a lot of arguments, which is an indicator that it could be doing too much or the input data is not related enough
2. mixed responsibilities: there are interface{} and provider-specific args in many helpers. Example: meta interface{} and schema.ResourceData args could be replaced with strong types like ConfiguredClient, retry/retry backoff integers, workspace ID, etc -- The idea would be that the helpers aren't interpreting provider schema or interfaces so that narrows the scope of their input.
3. the main entry point, createWorkspaceRun is looong so it may be best to try and break it into conceptual stages. I don't have a strong feeling about how to improve it, yet, but just moving code to subroutines that reflect the actual run stages might be a first step.

Does any of this resonate with you? What are your thoughts?

[Uk1288]

yes, this makes sense to me. I broke down the methods into smaller ones to represent action steps. The only one I have not updated is the double switch statement because the method is now smaller and straight forward. If we still want to get rid of that, I can replace it with an if..else statement. WDYT?

[Uk1288]

• TO-DO: upgrade go-tfe after changes are good to go.

[nfagerlund]

This is getting there! I think you've split out the duties in a fairly legible way among these functions; some of the argument lists are still a bit unpleasant, but I think that's destiny, kind of, when the task is this annoying.

But I have some major concerns about mutating that top-scope planPendingStatuses map, and I don't think we should do it. I think what I'd suggest for a refactor goes something like this:

The root issue:

• isPlanComplete() needs to inherently know about the list of plan terminal statuses.
• The call to awaitRun() needs to pass in the list of plan pending statuses.
• The actual lists of plan pending/terminal statuses depend on the run, plus whether hasPostPlanTaskStage (which is inherent in the run but has to be discovered by making additional requests and then remembered for later, and we don't want those tightly-scoped helper funcs hitting the client.)

What to do:

• Make a func that takes (run, hasPostPlanTaskStage) and returns a fresh planPendingStatuses map. (Which you'll later pass to awaitRun().)
• Make a func that takes (run, hasPostPlanTaskStage) and returns a fresh planTerminalStatuses map.
• Make a func that takes planTerminalStatuses and returns an anonymous function that can be used as an isPlanComplete() func.
• Then completely remove hasPostPlanTaskStage from the signatures of awaitRun and all the completion checker helper funcs, because isPlanComplete doesn't need it anymore -- it'll be a closure that already knows the things it currently needs hasPostPlanTaskStage in order to derive.

[nfagerlund]

Oh! Uh...... hmm. The planPendingStatuses map is defined at global scope in this package, which means that this is 🚨 mutating shared global state in the process of checking the run status. That freaks me out a bit! I can't remember whether the lifecycle of a provider server means we'll never be processing multiple runs concurrently, but this really feels like it's asking for trouble.

I DO like the logic of "sort out which statuses are actually pending or terminal based on the nature of the run we're working with," but I believe this requires each run to have its own list of pending statuses. (Or, like you already did with the terminal map, each invocation of the polling helper to have its own list.)

[Uk1288]

Right, mutating the global planPendingStatuses map raised questions for me as well, am glad that this comment validated my thought.

[nfagerlund]

So these revisions look pretty great. Once the required extra status is merged into go-tfe and the provider can build properly, hit me up if I'm around and I'll try and do a smoke test; I think I still have a config sitting around from the last time we jammed on this.

[nfagerlund]

All right!!! I smoke-tested this with the update to a released go-tfe version, and it works like a charm. We won't know for sure that this fixes the issue until we have the depth of randomness provided by the CI tests for some weeks, but I have a very good feeling about it at this point!!

We still need to fix the dang TFC runs API though 😆