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

v3: Don't count sequence indicator as part of indentation. #746

Closed

Conversation

monopole
Copy link

@monopole monopole commented Jun 9, 2021

This PR removes 2 spaces from sequence indentations to account for space consumed by the sequence item indicator, and adds tests around the use of the SetIndent api.

This maintains sequence indentation consistency with gopkg.in/yaml.v2 (see the example at https://pkg.go.dev/gopkg.in/yaml.v2#readme-api-documentation) and conforms to the comment:

both the “-” indicator and the following spaces are considered
to be part of the indentation of the nested collection.

in the specification at https://yaml.org/spec/1.2/spec.html#id2772075

Kubernetes is currently pinned to gopkg.in/yaml.v2, and kustomize (now a part of kubectl) is pinned to v3. We'd like the marshalling of sequences by kustomize and kubectl (e.g. kubectl create deployment foo -o yaml --dry-run --image bar) to match. This means reverting the parts of ae27a74 and a5ece68 that dealt with sequence indentation.

There's more background in kubernetes-sigs/kustomize#3946. There's time pressure as we'd like kubectl and kustomize to be consistent before the next code kubernetes code freeze in early July, and this PR or a fork seem like the only options.

@monopole monopole changed the title Sequence indicator shouldn't count as part of indentation. v3: Sequence indicator shouldn't count as part of indentation. Jun 9, 2021
@monopole
Copy link
Author

monopole commented Jun 9, 2021

This indentation reversion should also help with moving kubernetes-sigs/yaml to v3, see kubernetes-sigs/yaml#18

@monopole monopole changed the title v3: Sequence indicator shouldn't count as part of indentation. v3: Don't count sequence indicator as part of indentation. Jun 10, 2021
@monopole monopole force-pushed the sequenceIndicatorDoesntConsumeIndent branch 8 times, most recently from a282b02 to 99e927e Compare June 12, 2021 02:20
@monopole monopole force-pushed the sequenceIndicatorDoesntConsumeIndent branch 8 times, most recently from 9a609dc to ce9fc64 Compare June 14, 2021 20:16
@monopole monopole force-pushed the sequenceIndicatorDoesntConsumeIndent branch from ce9fc64 to 020d876 Compare June 14, 2021 20:29
@monopole
Copy link
Author

Abandoning this for now; the change always resulted in a few bad failures of existing or new tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant