Upgrade go-yaml

[Shell32-Natsu]

ALLOW_MODULE_SPAN

Upgrade the go-yaml v3 version used in kyaml to go-yaml/yaml@496545a. Almost all of the changes are indentions.

We have some YAML issues that are related to the bugs in go-yaml.

#3778
#3605
#3417
#3486

Upgrading go-yaml can also potentially fix

#3758
#3779

Meanwhile, some other tools which depend on kyaml, like kpt, will also obtain benefits from upgrading go-yaml.

Major change

The most significant (hopefully only) change is the indentation for list items. For example:

# before
list:
- a
- b
---
# after
list:
  - a
  - b

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Shell32-Natsu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Shell32-Natsu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Shell32-Natsu]

/test kustomize-presubmit-master

[Shell32-Natsu]

cc @droot

[Shell32-Natsu]

I think this is a pretty big change so we may need to merge this after releasing kustomize with new helm plugin. @monopole

[monopole]

This will break downstream tests.

The people who come looking for what happened will look at this PR.
The description probably needs some justification for the change
(e.g. point to bugs fixed).

[monopole]

and yes, after helm change :)

[natasha41575]

@monopole now that the helm change is merged and released, is this good to go?

[KnVerey]

@Shell32-Natsu did you investigate whether we have a way in kyaml (IIRC it has some style options?) that we could use to prevent the whitespace changes from surfacing in Kustomize? If you haven't looked into this, I'd be happy to.

Though potentially innocuous for many, this change might show up in some sense for a large portion of users. It could be disruptive for anyone with tests like ours, as well as anyone who is committing wet config (especially if they verify it against fresh builds in CI). Should this be part of a major version bump as a result?

Do we have tests proving that this PR fixes the issues mentioned?

Besides the whitespace, I see some dropped comments in this PR's diff. Were those manual or caused by the bump somehow?

[Shell32-Natsu]

@KnVerey I think there is no such way to configure the indentation in go-yaml.

I agree that this will break a lot of users' tests and this should be a major version release. Upgrading can fix the panic when there are non-ASCII characters like #3605 and #3617.

Besides the whitespace, I see some dropped comments in this PR's diff. Were those manual or caused by the bump somehow?

I think this is caused by bump.

[KnVerey]

If there's no way to get around the changes and it's going to trigger a major version bump, IMO we should have really solid evidence of the fixes it provides. I.e. I'd like to see a regression test (probably on a PR off this PR for reviewability) for each one of the issues cited in the description.

Re: the bump potentially causing dropped comments, that warrants investigation too.

[monopole]

Agree with @KnVerey.

I know it's a pain, but having pre-committed tests that repro the bugs fixed by the go-yaml bump, and hopefully show them to be on some normal usage path, would justify the need for this.

We should have those regardless. We ask the folks who file the issues to write such issue reproduction tests, tests that pass by showing bad behavior. Maybe they can help here?

This PR would then edit those tests, showing that the upgrade fixed the underlying issue.

For clarity, the test changes that are just indent adjustments could go in a commit by themselves.

[monopole]

briefly looked for a way to suppress the indent change, didn't find one.

Others want it, e.g.
go-yaml/yaml#202
go-yaml/yaml#720

The code to modify might be here:

https://github.com/go-yaml/yaml/blob/v3/encode.go#L483

There are some nice tests for encode which show the current (undesired) indentation.

[gautierdelorme]

@monopole according to go-yaml/yaml#661 the maintainer doesn't seem to be inclined to make it configurable.

[Shell32-Natsu]

Looks like the non-ascii issue has been addressed in #3827 because it bumped up the go-yaml version. Maybe we don't have a strong motivation to update the version to latest now. For kyaml users like kpt, they can manually pin go-yaml to newer version. Thoughts?

[KnVerey]

Looks like the non-ascii issue has been addressed in #3827 because it bumped up the go-yaml version.

That PR didn't bump the go-yaml version. kube-openapi would have brought along a more recent go-yaml version, but we opted to pin go-yaml to what Kustomize was already using to avoid the issues in this PR.

I don't think we need to bump go-yaml to latest right now on principle, but the open issues are serious. We should bump to whatever version fixes them, with tests to prove it does. If we pick up the whitespace changes in the process, the release it's in needs to be a major version bump. If not, it can be a regular release.

[monopole]

Since we're bound at the hip to kubectl, we cannot go higher than whatever is in

https://github.com/kubernetes/kubernetes/blob/master/go.mod#L492

(currently v3.0.0-20200615113413-eeeca48fe776).

If we go higher than that, we're obligated to do so in kubectl too. So an indentation change will have an even larger blast radius.

So yes @Shell32-Natsu, someone else can do manual replacements for now.

Eventually some critical bug will force either an indentation change across the board (kustomize, kubectl, etc) or a fork of gopkg.in/yaml.v3 to retain the current list indents.

[monopole]

Reopening per discussion in #3946. To keep things simple, we should try to land on the same version as in kubectl, so we should upgrade kubectl first.

[monopole]

Sorry to ask, but can you rebase? :P I could try next week if you don't have the time.

[Shell32-Natsu]

@monopole Rebased

[mikebz]

go-yaml/yaml#750 - awaiting review

[k8s-ci-robot]

@Shell32-Natsu: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.