Structured Renderer: use the new renderer when rendering the state in addition to the plan#32629
Conversation
liamcervante
force-pushed
the
liamcervante/renderer/state
branch
from
78f0b6a to
7e17e46
Compare
|
I could go through here and make a chain of smaller changes as I did before, if you think this PR is too big/complex to review in one. |
liamcervante
force-pushed
the
liamcervante/renderer/state
branch
from
7e17e46 to
b93ae7d
Compare
internal/command/jsonformat/state.go
Outdated
| renderer.Streams.Println() | ||
| } | ||
|
|
||
| // Render depth-first, so we show children first. |
There was a problem hiding this comment.
I don't understand the "so we show children first" part of this comment. At first I thought this block should be before the loop over resources in this module, but I think the behaviour we have is reasonable, it just doesn't seem to quite line up with the comment. Maybe I'm missing something.
There was a problem hiding this comment.
We don't want to show the children first. This is left over from confusion caused by the old approach.
Previously, the order of the modules was not consistent and AFAICT fairly random. And I was doing diff testing, and noticed in one run that my order was different and thought it was because I had root first and children after. I rearranged the order as described by you and then re-ran my checks and got a different result. So I decided that it makes more sense to show the root first as I had previously, and that we would now have a consistent ordering but I never went back and removed this comment 😓
I've removed the comment now, and since we're no longer making claims on the ordering I don't think a test is needed.
There was a problem hiding this comment.
I'm lacking some context here so this is just "FYI this exists" rather than any assumption it would be appropriate to use here, but I wanted to note that we have addrs.AbsResourceInstance.Less to define our "default ordering" for sets of resource instances where we don't have any other specific order to use.
That Less function takes into account various details, such as sorting first by module membership and then by resource address, and making sure that we use numeric ordering rather than string-based ordering for the integer instance keys caused by the count feature.
It looks like this code is currently using a locally-implemented order, I guess in part because it needs to take into account the possibility of "deposed objects" which are outside of what AbsResourceInstance can describe. But perhaps we could use the canonical ordering for any pair of addresses that have differences beyond the deposed key.
There was a problem hiding this comment.
Thanks for this Martin, that's some good context.
I'll leave this as is for now, because I think a better approach if we wanted to do the sorting properly would be to do the sorting in the json packages that generate the JSON structs themselves, and then the renderer can just print the things without sorting. I'll have a look at this in a separate PR.
For this PR, I think it's more important just to make sure the output is consistent and then we don't have to worry about converting back into the Terraform structs. If we do the sorting before we marshal it into JSON then it's already in the Terraform structs.
| Streams: v.view.streams, | ||
| Colorize: v.view.colorize, | ||
| Streams: v.view.streams, | ||
| RunningInAutomation: v.inAutomation, |
There was a problem hiding this comment.
Huh. I hadn't realized this wasn't hooked up until now. Nice catch!
| testSchemas(), | ||
| true, | ||
| "\n", | ||
| "The state file is empty. No resources are represented.\n", |
There was a problem hiding this comment.
This seems like a good change in behaviour, and our human-readable output is largely exempt from backwards compatibility guarantees, so I think it's fine. We should probably note it in the changelog, though.
There was a problem hiding this comment.
I'll update the changelog 👍
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
ghost
mentioned this pull request
|
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. |
3 participants
Fixes #32621
This PR adds an additional function to the new structured renderer:
RenderHumanState. This function is called from the show command so the state rendering and the plan rendering both use the same code path.There are a few changes to the diffing and rendering code to make this work.
HideDiffActionSymbols) that tells the renderer not to place any indents for symbols or actual symbols into the rendered output. This means the state rendering doesn't use the extra offsets placed in for the symbols that are used in the plan rendering. This is because the state renderer is always rendering no-ops so never has to show any symbols.ComputeChangeForTypefunction on the differ.Change objects is made public as the state renderer has more information about the types of outputs than the plan renderer and can use this function instead of interpreting the type.In addition to the above, I have refactored the renderer code so the plan and state rendering code paths are in their own files as the renderer.go file was getting very full. I think this makes the API of the renderer cleaner and clearer for callers. This change has no functional changes on anything and is just for aesthetics.
There are also numerous changes making the structs in the
jsonstatepackage public so the renderer can use them. This also involves making theIndexattributes intojson.RawMessageslices instead of keeping them as unparseable objects. This is an okay change, because they are rendered the same in JSON anyway and until now we were not reading them anywhere so the fact they couldn't be read back in was not a problem. Additionally, I update theAttributeValuestype so that it explicitly states that the values arejson.RawMessageslices. Previously, they were identified asinterface{}types but were always being populated asjson.RawMessageso I don't see why we don't just say that's what they are. This change also doesn't affect the actual output since they were alwaysjson.RawMessageobjects anyway.A future change could optimise the TFC backend so that it retrieves a JSONified version of the state and passes that directly into the renderer, but for now it can carry on retrieving the full state and converting to JSON locally as the local backend does anyway.