Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Support full fidelity YAML round-tripping #6489

Closed
wants to merge 2 commits into from

Conversation

lblackstone
Copy link
Member

@lblackstone lblackstone commented Mar 9, 2021

This change uses the new YAML parsing library to
to implement full fidelity round-tripping of config files.
Any comments and formatting will now be preserved
when Pulumi writes changes to disk.

Fix #423
Fix #6480
Fix #5235

TODO:

  • Swap out YAML package
  • Add logic to marshal/unmarshal YAML to AST
  • Update config set/rm to use AST
  • Add tests
  • Update config setAll/rmAll to use AST
  • Update Project and PolicyPackProject round-tripping (Going to follow up with this later)
  • Support structured config set/rm

@lblackstone lblackstone force-pushed the lblackstone/yaml-roundtrip branch 6 times, most recently from 5112c7d to d893394 Compare March 15, 2021 23:35
@lblackstone lblackstone force-pushed the lblackstone/yaml-roundtrip branch 14 times, most recently from 9b27bc7 to f42d0f0 Compare March 23, 2021 22:13
@lblackstone lblackstone force-pushed the lblackstone/yaml-roundtrip branch 5 times, most recently from b19369e to b16fc49 Compare March 29, 2021 22:10
@lblackstone lblackstone marked this pull request as ready for review March 31, 2021 00:00
This change uses the new YAML parsing library to
to implement full fidelity round-tripping of config files.
Any comments and formatting will now be preserved
when Pulumi writes changes to disk.
@squaremo
Copy link
Contributor

Rebasing this looks tractable -- is it worth bringing it up to date, @lblackstone? I can review it.

@lblackstone
Copy link
Member Author

Rebasing this looks tractable -- is it worth bringing it up to date, @lblackstone? I can review it.

Check out this comment about the state I left this in. The following comments in that thread also make me question whether the current duality of using the config as both user-editable as well as Pulumi-editable actually makes sense. I'll defer to @pulumi/platform on the latter question.

@AaronFriel
Copy link
Member

@Frassle I think this looks like a good candidate to pull in to this or the next iteration as we're considering adding another place where we mutate the stack config: seeding names.

@Frassle
Copy link
Member

Frassle commented Jun 13, 2022

The following comments in that thread also make me question whether the current duality of using the config as both user-editable as well as Pulumi-editable actually makes sense. I'll defer to

I don't think we're going to get away from that. If anything its going to get worse with ideas around yaml programs wanting to modify the Pulumi.yaml not just the Pulumi.<stack>.yaml.

@Frassle I think this looks like a good candidate to pull in to this or the next iteration as we're considering adding another place where we mutate the stack config: seeding names.

Yeh if this is close to done and doesn't have any major issues it would be good to get finished.

@AaronFriel
Copy link
Member

Implemented in #11456

@AaronFriel AaronFriel closed this Dec 13, 2022
@lukehoban lukehoban added this to the 0.81 milestone Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants