Implement YAML roundtripping, comment preserving edits #11456
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changelog[uncommitted] (2022-12-12)Features
|
AaronFriel
force-pushed
the
friel/yaml-roundtrip
branch
from
feb5cd6
to
c588409
Compare
AaronFriel
force-pushed
the
friel/yaml-roundtrip
branch
from
692988e
to
194eebf
Compare
iwahbe
approved these changes
Dec 8, 2022
sdk/go/common/util/yamlutil/edit.go
Outdated
Comment on lines
39
to
47
| var b bytes.Buffer | ||
| yamlEncoder := yaml.NewEncoder(&b) | ||
| yamlEncoder.SetIndent(2) | ||
| err = yamlEncoder.Encode(&newValue) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error editing value %#v: %w", newValue, err) | ||
| } | ||
|
|
||
| return b.Bytes(), nil |
There was a problem hiding this comment.
If there was a way to avoid repeating this in sdk/go/common/encoding/marshal.go, that would be great.
There was a problem hiding this comment.
Exported a two space indendation YamlEncode and refactored.
|
@iwahbe re: testing. Yeah I think so - we could modify an integration test to start with a file that has some config and comments. I'll do that. |
AaronFriel
force-pushed
the
friel/yaml-roundtrip
branch
2 times, most recently
from
27c750e
to
166a652
Compare
|
@Frassle @justinvp @iwahbe Added tests here: https://github.com/pulumi/pulumi/blob/friel/yaml-roundtrip/tests/roundtrip_test.go I'd like the first test to use |
AaronFriel
force-pushed
the
friel/yaml-roundtrip
branch
2 times, most recently
from
a5c3d75
to
39a108e
Compare
AaronFriel
force-pushed
the
friel/yaml-roundtrip
branch
3 times, most recently
from
176cf85
to
12a8743
Compare
|
bors merge |
bors bot
added a commit
that referenced
this pull request
Dec 13, 2022
11456: Implement YAML roundtripping, comment preserving edits r=AaronFriel a=AaronFriel By caching a copy of the raw bytes of the original loaded document - project, policy project, or stack - on save we: * Unmarshal the original bytes to a YAML AST * Recursively edit nodes of the document to match the values of the value we're saving, preserving trivia * Marshal the modified AST to []byte Closes #423, as we don't support comments in JSON presently (not using "JSON5") Closes #5235 Simplifies work done in #10797 and #10437 which used the previous `yamlutil` package to edit AST nodes directly. # Automation API Integration with automation API requires that operations either perform a "read-modify-write" in a single operation, or use a side channel to allow subsequent "read" (select stack?) and "write" operations to keep state of the original file. However I don't expect this to be needed. We don't have a reason to believe automation API users maintain comments in particular stack files, and doing so is already unsupported in automation scenarios. The Pulumi Service does not persist raw config files, thus `pulumi config refresh` and `CreateOrSelectStack` create config files without comments. n.b.: If you test this, due to the new behavior of this PR, `pulumi config refresh` will persist comments if there is an existing config file. I would expect automation API using local file workspaces to behave similarly. When using these tools to create a config file from the last deployment's definition, the file is created restored without comments. # Tradeoffs I believe that this closes #423 in spirit, but doesn't strictly implement round-tripping. There are a couple reasons for this. ## Marshaller interface and round-tripping simplification decisions We support unmarshaling "simplified" YAML, including allowing: ```yaml runtime: yaml # this unmarshals to a struct, not a scalar value config: someKey: "someScalar" # likewise ``` These simplifications make it much more difficult, the `Marshal` operation would _also_ need context of decisions made when unmarshaling to ensure a bijection. Otherwise we may save one of these as the other: ```yaml runtime: name: yaml ``` ```yaml runtime: yaml ``` This could be considered a feature, as it enforces a style guide - the decisions implemented in our custom `Marshaller` implementations become the style enforced on save. That said, I believe the new `Edit` API while not round-trippable, does reach a fixed point after one save through the new API. Once saved with this API, subsequent loads & saves without changes are idempotent. ## Indentation The YAML library uses a global indentation when marshaling values to YAML, as opposed to indentation being a property of AST nodes. This means we may lose other indentation. Co-authored-by: Aaron Friel <mayreply@aaronfriel.com>
7 tasks
|
Build failed: |
|
bors retry |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By caching a copy of the raw bytes of the original loaded document - project, policy project, or stack - on save we:
Closes #423, as we don't support comments in JSON presently (not using "JSON5")
Closes #5235
Simplifies work done in #10797 and #10437 which used the previous
yamlutilpackage to edit AST nodes directly.Automation API
Integration with automation API requires that operations either perform a "read-modify-write" in a single operation, or use a side channel to allow subsequent "read" (select stack?) and "write" operations to keep state of the original file.
However I don't expect this to be needed. We don't have a reason to believe automation API users maintain comments in particular stack files, and doing so is already unsupported in automation scenarios. The Pulumi Service does not persist raw config files, thus
pulumi config refreshandCreateOrSelectStackcreate config files without comments.n.b.: If you test this, due to the new behavior of this PR,
pulumi config refreshwill persist comments if there is an existing config file. I would expect automation API using local file workspaces to behave similarly. When using these tools to create a config file from the last deployment's definition, the file is created restored without comments.Tradeoffs
I believe that this closes #423 in spirit, but doesn't strictly implement round-tripping. There are a couple reasons for this.
Marshaller interface and round-tripping simplification decisions
We support unmarshaling "simplified" YAML, including allowing:
These simplifications make it much more difficult, the
Marshaloperation would also need context of decisions made when unmarshaling to ensure a bijection. Otherwise we may save one of these as the other:This could be considered a feature, as it enforces a style guide - the decisions implemented in our custom
Marshallerimplementations become the style enforced on save.That said, I believe the new
EditAPI while not round-trippable, does reach a fixed point after one save through the new API. Once saved with this API, subsequent loads & saves without changes are idempotent.Indentation
The YAML library uses a global indentation when marshaling values to YAML, as opposed to indentation being a property of AST nodes. This means we may lose other indentation.