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 round-tripping of Pulumi.[yaml|json] #423

Closed
Tracked by #4604
ellismg opened this issue Oct 17, 2017 · 11 comments · Fixed by #11456
Closed
Tracked by #4604

Support full fidelity round-tripping of Pulumi.[yaml|json] #423

ellismg opened this issue Oct 17, 2017 · 11 comments · Fixed by #11456
Assignees
Labels
area/cli UX of using the CLI (args, output, logs) kind/enhancement Improvements or new features resolution/fixed This issue was fixed
Milestone

Comments

@ellismg
Copy link
Contributor

ellismg commented Oct 17, 2017

When writing information to the Pulumi.yaml file, we end up serializing back the entire structure and that serialization works by serializing each field in the pack.Package in the order it was defined in the go object.

In the case of YAML any comments would also be removed from the file.

This means if we had a Pulumi.yaml file that looked like this:

runtime: nodejs
name: hello-world

If we read and save it it would look like this:

name: hello-world
runtime: nodejs

Ideally, we would have parsers that would turn these files not into raw go objects but some DOM that we could round trip with full fidelity.

@ellismg ellismg added area/cli UX of using the CLI (args, output, logs) kind/enhancement Improvements or new features good-first-issue Start here if you'd like to start contributing to Pulumi labels Oct 17, 2017
@joeduffy
Copy link
Member

Seemingly relevant: go-yaml/yaml#132.

@justinvp
Copy link
Member

justinvp commented Dec 5, 2019

Applies to Pulumi.<stack>.yaml as well.

@lblackstone
Copy link
Member

lblackstone commented Mar 4, 2021

I've looked into this and can't find a YAML parser that supports full-fidelity round-tripping. It appears that the goyaml/v3 package comes closest, but their docs include the following note:

It's worth noting that although Node offers access into details such as line numbers, colums, and comments, the content when re-encoded will not have its original textual representation preserved. An effort is made to render the data plesantly, and to preserve comments near the data they describe, though.

With this in mind, we should at least be able to preserve the comments in the config files, but things may still move around a bit. This would also require upgrading from v2 to v3 of this package.

@lblackstone
Copy link
Member

Have made good progress on this issue since my last update. After some more experimentation, we determined that the github.com/goccy/go-yaml package is suitable for this task with some additional tweaks. We forked that library to github.com/pulumi/go-yaml, and made the required changes to support full-fidelity round-tripping (preserving comments, spacing, etc.).

I have a proof of concept project working locally that can unmarshal a Pulumi config, add (or delete) a config value, and then marshal it back to YAML.

yamlRoundtrip

The next steps are to update the YAML unmarshaling/marshaling code in the pulumi CLI to use this package.

@leezen leezen modified the milestones: 0.53, 0.54 Mar 16, 2021
lblackstone added a commit that referenced this issue Mar 29, 2021
Some YAML parsers don't correctly handle Byte-order Marks,
so automatically strip it off during load.

Related to #423
lblackstone added a commit that referenced this issue Mar 29, 2021
Some YAML parsers don't correctly handle Byte-order marks,
so automatically strip it off during load.

Related to #423

Co-authored-by: Justin Van Patten <jvp@justinvp.com>
lblackstone added a commit that referenced this issue Mar 29, 2021
lblackstone added a commit that referenced this issue Mar 29, 2021
@infin8x infin8x removed this from the 0.54 milestone Apr 2, 2021
@joeduffy joeduffy added the pj2 label Jul 10, 2021
@lblackstone
Copy link
Member

Quick update on status of this issue:

This is currently marked as "blocked" because some unforeseen compatibility issues with the new YAML parser library required us to revert the PR to add round-tripping support. The work to fix these issues was preempted by some other engineering priorities, so it's constrained by engineering investment rather than a specific issue.

The actual round-tripping work is basically done, but we need to do some more work to make sure that the YAML marshaling/unmarshaling is compatible with the existing library. Once that is done, we need to swap out the libraries again and then merge the in-progress PR.

@filip-zyzniewski
Copy link

filip-zyzniewski commented Oct 19, 2021

The funny side effect of this is that https://app.pulumi.com/ complains that the git checkout used for a preview was dirty:

Given that this preview ran from a CI job and that we have comments in our stack configs, I assume that the complaint is a result of overwriting the stack config.

@atrauzzi
Copy link

atrauzzi commented Nov 12, 2021

So, I'm gonna throw a radical idea out there: But has Pulumi considered simply not touching configuration files unless explicitly asked?

I talk a little bit about this odd behaviour in this ticket and also here.

Pulumi seems to expect users to manage the config files, but also seems to want to treat it as an extension of its internally managed state store. Is there any way Pulumi can just be a reader of files in source control and nothing else?

@AaronFriel AaronFriel self-assigned this Mar 17, 2022
@AaronFriel AaronFriel removed the good-first-issue Start here if you'd like to start contributing to Pulumi label Jun 14, 2022
@AaronFriel AaronFriel added this to the 0.82 milestone Dec 10, 2022
@AaronFriel
Copy link
Member

Comment-preserving edits will be implemented in the next release of Pulumi.

@AaronFriel
Copy link
Member

AaronFriel commented Dec 10, 2022

@atrauzzi Our users find pulumi config --set and assorted commands to be very helpful, as a scriptable interface to managing this aspect of Pulumi. I'm pleased to say that the next release will preserve comments when using commands that edit project and config files.

When editing a file like so:

encryptionsalt: v1:ThS5UPxP9qc=:v1:UZYAXF+ylaJ0rGhv:9OTvBnOEDFgxs7btjzSu+LZ470vLpg==
# 🔴 header comment
config:
  foo:a: some-value # 🟠 comment after value
  # 🟡 comment before key
  foo:b: some-value
  foo:c:
    a: A
    b: B
    c:
      - 1
      - 2
      - 3 # 🟢 comment in array
      # 🔵 comment after array
  foo:d:
    secure: v1:T1ftqhY0hqr+EJK6:+jvd5PMecFx80tcavzuZY4tLatgIfoe/xR72GA== # 🟣 comment on secret

# 🟥 footer comment

After running pulumi config set e E, and pulumi config set --path c.c[2] three the file will contain:

encryptionsalt: v1:ThS5UPxP9qc=:v1:UZYAXF+ylaJ0rGhv:9OTvBnOEDFgxs7btjzSu+LZ470vLpg==
# 🔴 header comment
config:
  foo:a: some-value # 🟠 comment after value
  # 🟡 comment before key
  foo:b: some-value
  foo:c:
    a: A
    b: B
    c:
      - 1
      - 2
      - three # 🟢 comment in array
      # 🔵 comment after array
  foo:d:
    secure: v1:T1ftqhY0hqr+EJK6:+jvd5PMecFx80tcavzuZY4tLatgIfoe/xR72GA== # 🟣 comment on secret
  foo:e: E

# 🟥 footer comment

bors bot added a commit that referenced this issue 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>
@bors bors bot closed this as completed in 7a173be Dec 13, 2022
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Dec 13, 2022
@jamest-pin
Copy link

jamest-pin commented Jan 9, 2024

The conversation in this collection of linked issues seems to read that anchors are supported in Pulumi.yaml

But, I tried using them:

# Pulumi.yaml

name: my-app
runtime: yaml
description: my app

# template
tags: &defaultTags
  Environment: ${pulumi.stack}
  CreatedBy: pulumi

resources:
  ####################################
  # L O G   G R O U P S
  # --------------------------------
  # https://www.pulumi.com/registry/packages/aws/api-  docs/cloudwatch/loggroup/
  ####################################

  # template for log groups
  logGroupTemplate: &logGroupTemplate
    type: aws:cloudwatch:LogGroup
    properties:
      tags:
        <<: *defaultTags
      logGroupClass: STANDARD
      retentionInDays: 731
      skipDestroy: True # leave logs there in case we want to go back to them. They'll fall off after the retention period

  # log group for lambda admin auth
  lg_lambda_admin_auth:
    <<: *logGroupTemplate

and am getting

error: failed to discover plugin requirements: 
-error: Pulumi.yaml:1086,13-24: alias nodes are not supported; 
-error: Pulumi.yaml:1093,9-25: alias nodes are not supported; 

Is it supposed to work or do I need to submit a new feature request?

@AaronFriel
Copy link
Member

AaronFriel commented Jan 23, 2024

@jamest-pin Sorry for the delay in replying! This issue was closed and often closed issues don't get a lot of attention. You may want to open a new issue - it looks like this would belong on github.com/pulumi/pulumi-yaml.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli UX of using the CLI (args, output, logs) kind/enhancement Improvements or new features resolution/fixed This issue was fixed
Projects
None yet