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

Pulumi Should retain comments in Stack Config #5235

Closed
JasonWhall opened this issue Aug 27, 2020 · 8 comments · Fixed by #11456
Closed

Pulumi Should retain comments in Stack Config #5235

JasonWhall opened this issue Aug 27, 2020 · 8 comments · Fixed by #11456
Labels
area/config pulumi config kind/enhancement Improvements or new features resolution/fixed This issue was fixed

Comments

@JasonWhall
Copy link
Contributor

JasonWhall commented Aug 27, 2020

When running Pulumi commands like pulumi up or pulumi preview when using Azure KeyVault as it's Secrets Provider for the selected stac, comments in the relevant config file are removed. It would be useful to retain these comments as config files could be edited by hand and checked into source control with these comments to provide information to other users about the reasoning for the config value. When not using the --secrets-provider flag. Running pulumi commands retain comments in the config as expected.

Example stack config file pulumi.<stack>.yaml:

# Top level comment
secretsprovider: REDACTED
encryptedkey: REDACTED
config:
  # Another Comment
  MyProject:GroupIds: 
  - 11111111-1111-1111-1111-111111111111 # Group one
  - 11111111-1111-1111-1111-111111111112 # Group 2

Expected Behavior

Comments should be retained in the config file as these could contain useful information that refers to config values

Current Behavior

All comments are removed from config

Steps to Reproduce

  1. Create a new project pulumi new csharp
  2. Initialise the stack with Azure KeyVault as the secrets provider
pulumi stack init dev --secrets-provider=azurekeyvault://<Vault Name>.vault.azure.net/keys/<Key Name>
  1. Open the pulumi.dev.yaml file and add a comment
  2. Run pulumi up and comments will be removed
@JasonWhall JasonWhall added the needs-triage Needs attention from the triage team label Aug 27, 2020
@lukehoban
Copy link
Member

@JasonWhall Can you confirm which version of the pulumi CLI you were using with pulumi version?

@JasonWhall
Copy link
Contributor Author

@lukehoban - Apologies I've just re-tested this and the issue only occurs when using the stack has a --secrets-provider. I've reproduced this using Azure KeyVault as the secrets provider but may affect when using other providers as well.

I'm using latest Pulumi CLI v2.9.0

I've also updated the steps to reproduce to reflect this.

@lukehoban
Copy link
Member

@stack72 is this something we can return back to pre-2.8.0 behavior without a larger change to YAML serialization?

@leezen leezen added this to the current milestone Sep 8, 2020
@leezen leezen removed this from the current milestone Nov 2, 2020
@alastairs
Copy link

Any update on this one? I've used comments in the stack files to annotate the Azure subscription IDs with their names, and was disappointed to find that all lost as soon as I ran a pulumi preview.

@farvour
Copy link

farvour commented May 13, 2021

Hmm, we use the hashivault secrets provider and have this problem as well.

Pulumi v2.21.2 at the time of this writing.

@austinbutler
Copy link

Still an issue with 3.4.0. Pulumi also likes to reformat the file even when there are no changes, which is annoying if you have a prettier pre-commit hook.

@leezen leezen removed the needs-triage Needs attention from the triage team label Jun 23, 2021
@emiliza emiliza added area/config pulumi config kind/enhancement Improvements or new features labels Nov 30, 2021
@1oglop1
Copy link

1oglop1 commented Jun 14, 2022

Any chance this can be picked up soon? It is extremely annoying if you actually use stack configs :(

@Frassle
Copy link
Member

Frassle commented Jun 14, 2022

Was suggested today to see about bringing #6489 up to date and giving that another try which would fix this issue.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config pulumi config kind/enhancement Improvements or new features resolution/fixed This issue was fixed
Projects
None yet