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

Make kyaml indentation style configurable #4029

Closed
natasha41575 opened this issue Jul 1, 2021 · 3 comments · Fixed by #4043
Closed

Make kyaml indentation style configurable #4029

natasha41575 opened this issue Jul 1, 2021 · 3 comments · Fixed by #4043
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@natasha41575
Copy link
Contributor

natasha41575 commented Jul 1, 2021

Kyaml is currently using a version of the go-yaml library that uses the old indentation style for sequence nodes, allowing us to get fixes from the upstream library without accepting the new indentation. The new indentation style is hardcoded as part of the NewEncoder method in kyaml.

This presents problems for other tools which are using kyaml as a low-level yaml processing library. For example, kpt had made the decision to go with the new indentation style for beta launch and are now unable to upgrade to kyaml v0.11.0, which is blocking them from moving forward. We should make the indentation style configurable in kyaml for the same reason that we needed the indentation style to be configurable in go-yaml: to prevent breaking whitespace changes from library consumers. This will help them have minimum future disruption to users and it is in the best interest of everyone to have one library that can be used by multiple tools and is maintained by both teams rather than requiring them to make separate forks.

The configurable indentation style should allow library consumers to decide whether to use the compact sequence indentation style where "- " is considered part of the indentation, or the wide sequence indentation style where "- " is not considered part of the indentation.

Potential options:

  1. Kyaml can add public methods to set the indentation style (this would go in kyaml/yaml/alias.go).
  2. kyaml.NewEncoder and kyaml.Marshal can take parameters to set the indentation style.
  3. We can allow NewEncoder to return an interface that has all the methods of an Encoder, so that kpt can override it to directly use the upstream yaml.v3 Encoder.

Of all these, I prefer option 1, because it is the simplest.

@natasha41575 natasha41575 added kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Jul 1, 2021
@natasha41575
Copy link
Contributor Author

@KnVerey
Copy link
Contributor

KnVerey commented Jul 6, 2021

We should make the indentation style configurable in kyaml for the same reason that we needed the indentation style to be configurable in go-yaml: to prevent breaking whitespace changes from library consumers.

I agree with you. The upstream change was out in the wild for a very long time, and it would have been reasonable for library consumers and their users to have already done a painful migration, not realizing that kyaml would restore the original behavior in the future. Although kyaml itself is in alpha, this change affects not only tool builders using it, but all their end users--just like our own. As a result, I think adding a configuration option is the empathetic thing to do. That said, I also agree with previous points raised that we should be wary of exposing a wide array of tuneable parameters, leading to an explosion of combinations that need to be tested as well as potentially complicating our dependency on the underlying yaml lib. Despite this particular decision, let's generally continue to favour being opinionated over configurable (Golang style) going forward. Also of note, we should reserve the right to change the details of the parameters we expose to be more closely aligned with the upstream change that eventually goes in, if necessary (that would be a backwards-incompatible change for library users, but not end users, which is acceptable while kyaml is in alpha).

Between the three options mentioned, I also prefer 1. Option 3 strikes me as more complex than necessary, and option 2 would be a divergence with the parameters of the underlying functions we're wrapping.

@natasha41575
Copy link
Contributor Author

/assign @phanimarupaka

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants