DO NOT MERGE: demonstrate required changes for yaml update

[natasha41575]

used the fork of yaml from this PR go-yaml/yaml#750 and updated the kyaml tests so that they pass

EDIT: Now points to the fork in go-yaml/yaml#753

tracking issue: #3946

this PR demonstrates what impact the change would have in kyaml

cc @mikebz

ALLOW_MODULE_SPAN

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natasha41575

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 [natasha41575]

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

[k8s-ci-robot]

@natasha41575: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

[KnVerey]

Are any changes required outside kyaml? Can you push the replace statement so that we can see the test suite state too?

[natasha41575]

/retest

[KnVerey]

Having examined all the changes in this PR, here's what I conclude so far.

There are two things that need to be addressed on @mikebz PR:

• Non-indent style sequences don't seem to be respecting custom indent. To see this, write a test with SetIndent(4) as well as SetIndentSequences(false).
• Something very strange happens with nested lists, per https://github.com/kubernetes-sigs/kustomize/pull/4004/files?file-filters%5B%5D=.go&file-filters%5B%5D=.yaml#r653803898.

There are also two potentially concerning changes from the intermediate diff:

• Feature (v3): set the default line length to infinity (-1) go-yaml/yaml#572. I think this is a desirable improvement, and some may even consider it a bug fix. But is is a change that will cause a diff for some users, and we will need to make a judgement call on whether that warrants a major version bump for the CLI.
• The apparent loss of a newline from the source file here: https://github.com/kubernetes-sigs/kustomize/pull/4004/files?file-filters%5B%5D=.go&file-filters%5B%5D=.yaml#diff-a497b67d0a86cac8b63450bff25432f185a5bab7413ba04f67212a7d36241cffL185. Still need to dig further into this one.

[natasha41575]

Examples of what happens now:

1. Default (indent=2, sequence indents disabled):

apiVersion: config.kubernetes.io/v1alpha1
kind: ResourceList
items:
- kind: Deployment
  apiVersion: v1
  metadata:
    name: tester
    namespace: default
  spec:
    replicas: 0
results:
  name: Incompatible config
  items:
  - message: bad value for replicas
    severity: error
    resourceRef:
      apiVersion: v1
      kind: Deployment
      name: tester
      namespace: default
    field:
      path: .spec.Replicas
      currentValue: "0"
      suggestedValue: "3"
    file:
      path: /path/to/deployment.yaml
  - message: some error
    severity: error

2. Default indent (=2), EnableSeqIndent():

apiVersion: config.kubernetes.io/v1alpha1
kind: ResourceList
items:
  - kind: Deployment
    apiVersion: v1
    metadata:
      name: tester
      namespace: default
    spec:
      replicas: 0
results:
  name: Incompatible config
  items:
    - message: bad value for replicas
      severity: error
      resourceRef:
        apiVersion: v1
        kind: Deployment
        name: tester
        namespace: default
      field:
        path: .spec.Replicas
        currentValue: "0"
        suggestedValue: "3"
      file:
        path: /path/to/deployment.yaml
    - message: some error
      severity: error

Note: the above is the same output that you get if you use yaml.v3 at head with SetIndent(2)

3. SetIndent(4), use default sequence indent (disabled sequence indents)

apiVersion: config.kubernetes.io/v1alpha1
kind: ResourceList
items:
  - kind: Deployment
    apiVersion: v1
    metadata:
        name: tester
        namespace: default
    spec:
        replicas: 0
results:
    name: Incompatible config
    items:
      - message: bad value for replicas
        severity: error
        resourceRef:
            apiVersion: v1
            kind: Deployment
            name: tester
            namespace: default
        field:
            path: .spec.Replicas
            currentValue: "0"
            suggestedValue: "3"
        file:
            path: /path/to/deployment.yaml
      - message: some error
        severity: error

4. SetIndent(4) and EnableSeqIndent():

apiVersion: config.kubernetes.io/v1alpha1
kind: ResourceList
items:
    - kind: Deployment
      apiVersion: v1
      metadata:
        name: tester
        namespace: default
      spec:
        replicas: 0
results:
    name: Incompatible config
    items:
        - message: bad value for replicas
          severity: error
          resourceRef:
            apiVersion: v1
            kind: Deployment
            name: tester
            namespace: default
          field:
            path: .spec.Replicas
            currentValue: "0"
            suggestedValue: "3"
          file:
            path: /path/to/deployment.yaml
        - message: some error
          severity: error

Note: the above is the same output that you get if you use yaml.v3 at head with SetIndent(4)

[KnVerey]

Awesome work on this! No concerns with what's left in the diff now.

[phanimarupaka]

Comments are reordered here. Why?

[KnVerey]

I think this is a bug fix. If you look at the input fixture above, the comments are in the new position, not the old:

- apiVersion: v1
  kind: Service
  metadata:
    name: test
    labels:
      app: nginx
      name: test
  spec:
    ports:
    # This i the port.
    - port: 8080
      targetPort: 8080
      name: http
    selector:
      app: nginx
      name: test

[phanimarupaka]

@natasha41575 I tried your fork and here are the couple of issues I noticed.

1. Head comments are being deleted.
   In the example I tried, there is --- after head comment which makes the parse think it is empty node and drops the comment. I think this is expected and nothing much can be done.
2. Multi-line strings are converted to single line. I think there is already an issue in go-yaml about this.

Can you please verify and confirm?

[KnVerey]

Multi-line strings are converted to single line. I think there is already an issue in go-yaml about this.

Are you referring to the changes visible in this PR? If so, the strings are not multi-line in the input. Previously, go-yaml wrapped after a certain string length, and the change is that it intentionally stopped doing that: go-yaml/yaml#572. Natasha and I discussed whether this means we need a new major version of Kustomize, but she found a previous case where essentially the same thing changed in a minor release in the past. If you're referring to something else, please provide an example.

Head comments are being deleted. In the example I tried, there is --- after head comment which makes the parse think it is empty node and drops the comment. I think this is expected and nothing much can be done.

I agree that's what I'd expect: if there are comments before a --- document header, then they technically belong to a previous empty document and will be dropped. Did this behave differently with previous versions of kyaml?

[natasha41575]

The commits here were merged in #4013