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

Breaking whitespace changes in go-yaml upgrade #3946

Closed
KnVerey opened this issue Jun 2, 2021 · 12 comments
Closed

Breaking whitespace changes in go-yaml upgrade #3946

KnVerey opened this issue Jun 2, 2021 · 12 comments
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@KnVerey
Copy link
Contributor

KnVerey commented Jun 2, 2021

As discovered in #3789, upgrading go-yaml causes whitespace changes in the output. This may be seriously disruptive to some users, as committing build results is a common practice for both testing and GitOps workflows.

We need to either:
A) Prevent the whitespace changes. Presumably this involves kyaml interfering with the marshalling process. Related to #3559
B) Release the go-yaml bump in a major version bump (i.e. kustomize 5.0).

This issue is urgent because although we can pin the version used by the standalone kustomize binary, kubernetes/kubernetes has already upgraded to a more recent version. This presumably means the next kubectl release will include the breaking changes as of right now (TBC).

@KnVerey KnVerey added the kind/bug Categorizes issue or PR as related to a bug. label Jun 2, 2021
@KnVerey
Copy link
Contributor Author

KnVerey commented Jun 2, 2021

/priority important-soon

@Shell32-Natsu I couldn't find an issue for this outside your PR, but I believe you were working on this. Are you still?

@k8s-ci-robot k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jun 2, 2021
@monopole
Copy link
Contributor

monopole commented Jun 5, 2021

This issue will probably get some interest/questions down the road, so adding some history.

In a commit on 2020-May-04, marshaling code in gopkg.in/yaml.v3 changed indenting from

favorites:
  fruit: apple
  car: ford
fruits
- apple
- orange

to

favorites:
  fruit: apple
  car: ford
fruits
  - apple
  - orange

Kustomize (via kyaml) depends on gopkg.in/yaml.v3. Hundreds of kustomize tests assume the older more compact behavior, where the hyphens don't 'consume' an indent. These tests are intentionally brittle in this fashion as downstream consumers of the YAML also compare differences in marhsalled YAML and don't want surprises.

The modules in this repo (kyaml and api) use replace directives to pin to the commit just before this indentation change (back to 2020-Mar-13).

Meanwhile,

  • kubectl depends on sigs.k8s.io/yaml, which in turn depends on gopkg.in/yaml.v2, which has the compact indent. One can see this by using the command kubectl create deployment foo --dry-run -o yaml --image foo
  • The k8s repo has gopkg.in/yaml.v3 dependence, but not via kubectl.
  • There have been commits to go-yaml fixing various bugs and improving YAML comment retention; kyaml clients (including kustomize) need those fixes.
  • Although we could send a PR into go-yaml exposing a means to pick an indentation style, that's exactly the kind of knob that the Go community would never add to the gofmt tool because it can lead to a mess

Options

  1. Dust off Upgrade go-yaml #3789, which accepts the indentation change (and updates tests), then release kustomize v5 with the indentation change and make a bunch of noise about it for folks that have downstream tests to adjust. This means kustomize and kubectl will differ in their marshalling.

  2. Fork gopkg.in/yaml.v3, and remove the indentation change. Our users won't have to adapt to a change, and kustomize will marshal like kubectl (modulo other differences between v2 and v3).

  3. Send a PR to gopkg.in/yaml.v3 that undoes the indent-changing commit, hope it is accepted.

edit: tune up comments about kubectl behavior after investigating how the k8s repo used v2 vs v3

@monopole
Copy link
Contributor

monopole commented Jun 5, 2021

The upgrade of k8s to yaml.v3 circa 2020-Jun-15 happened in kubernetes/kubernetes#100490 on 2021 Apr 20 - just a few weeks ago.

Were the dozen or so later commits to yaml.v3 that this PR could have grabbed intentionally kept out? Meaning step 1 above wouldn't make it past code rev?

@howardjohn
@gautierdelorme

@monopole
Copy link
Contributor

monopole commented Jun 5, 2021

Some of the issues in various repos related to this:

@stefanprodan
Copy link

@monopole for Flux users, I think it's best to autodetected and keep the original indentation, unlike go fmt, Kubernetes users are not accustom to strict formatting since kubectl does no such enforcement.

@KnVerey
Copy link
Contributor Author

KnVerey commented Jun 9, 2021

After further thought, we're exploring both options 2 and 3 @monopole mentioned above. Option 1 is likely a no-go even if we were to cut a new major version of Kustomize, because of the issue of inconsistency with kubectl, which is definitely not able to change.

@monopole
Copy link
Contributor

monopole commented Jun 9, 2021

The PR for option 3 is go-yaml/yaml#746

@KnVerey
Copy link
Contributor Author

KnVerey commented Jun 9, 2021

/assign @monopole @KnVerey

@frankfarzan
Copy link
Contributor

/cc @phanimarupaka

@mikebz
Copy link
Contributor

mikebz commented Jun 17, 2021

go-yaml/yaml#750

@natasha41575
Copy link
Contributor

go-yaml/yaml#753

@natasha41575
Copy link
Contributor

Closing this in favor of #4033, which includes a summary of the decision made and what we need to do moving forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

No branches or pull requests

7 participants