v3 - Configurable or autodetected list indentation

[seboudry]

Hi!

When using v3, identation on list child are not preserved. That was the case in v2.

Input:

list:
- foo: bar

Expected:

list:
- foo: bar

Actual (based on encoder indent):

list:
    - foo: bar

[mikebz]

@niemeyer can we provide some help in making this configurable? Seems like this change didn't make it in: #746
Not sure if this is because goyaml is trying to be opinionated around formatting.

It would be nice to provide some continuity to folks who are updating from v2 to v3, so the upgrade doesn't mean you also have to update all your tests (and possibly your customers' tests)

[niemeyer]

@mikebz Yes, that's indeed the default indentation for lists now. We can have a setting if that improve things for migration, though. I'll need to sort out that PR for having options more conveniently first, though. It's already in my TODO list for the next batch of work here.

[natasha41575]

@niemeyer Thanks for working on this! I was wondering if you have a timeline of when you think this will be available? We are on a bit of a tight schedule.

[niemeyer]

@natasha41575 What's your timeline for this?

[natasha41575]

We would like to get an upgrade into the kubernetes repo before code freeze if possible - July 8th.

[niemeyer]

If I find a slot to get this through before the end of the month, does that give you enough time to land it? Do you have a PR on your end with everything else ready besides this issue?

[mikebz]

We might build on @monopole 's PR but we wanted to check with you first before exerting a lot of effort. Seems like there is a green light so we can start the work 🟢

[natasha41575]

If I find a slot to get this through before the end of the month, does that give you enough time to land it? Do you have a PR on your end with everything else ready besides this issue?

We aren't sure yet if that's enough time but hopefully it will be. Thanks for getting back to us. Echoing @mikebz we are happy to contribute if needed.

[niemeyer]

If you aren't sure it becomes a bit less important for me to prioritize it, so please drop me a note if you have more details of your own schedule.

[mikebz]

@niemeyer here is the schedule: https://github.com/kubernetes/sig-release/tree/master/releases/release-1.22 the issue discussed today in SIG CLI (kustomize, kubectl) is that v3 was imported already into kubectl, but when it's updated in kustomize it's impacting existing v2 customers. Kustomize also gets merged into kubectl to enable kubectl -k so the same version of the library needs to be across the board.

We will need to have a PR and a release done for goyaml in advance of that date or we will need to downgrade the kubectl libraries back to v2.

[niemeyer]

I see, so if I understand it correctly you're already on v3 and assuming we get an option that supports the old indentation nothing is required other than updating it and turning the flag on. Is that right?

If that's the case, then I'm happy to attempt to get that in by the end of the month.

[natasha41575]

you're already on v3 and assuming we get an option that supports the old indentation nothing is required other than updating it and turning the flag on. Is that right?

Yes, that's correct. We've just confirmed with a kubernetes dependency maintainer that it shouldn't be a problem to update if this change is in by the end of the month. Thanks so much!

[mikebz]

@niemeyer question for you. Is your preference to have a formatting flags structure that can be used to set various formatting preferences in the future or a one for flag for this situation. You have a way to set indent length, already

// SetIndent changes the used indentation used when encoding.
func (e *Encoder) SetIndent(spaces int) {
	if spaces < 0 {
		panic("yaml: cannot indent to a negative number of spaces")
	}
	e.encoder.indent = spaces
}

[niemeyer]

@mikebz We have some previous discussions and pending work already to add an options type in various contexts, so I'd rather not extend the methods we have at the moment on the encoder/decoder if possible, so that we have less to obsolete and break later. I'll look into this at the same time.

[mikebz]

@niemeyer sounds good, let me know when you have some feedback or if you have links to issues/PRs that are solving the same problem. One argument could be that this is no different than setting the indent width, but of course if we anticipate a larger set of formatting flags it might be better to come up with a structure.

[natasha41575]

@niemeyer Wanted to reach out to see if there are any updates on this. We're hoping it'll be possible for us to do a release early next week with this change.

[mikebz]

@niemeyer I wanted to echo what @natasha41575 said above. I think it would be great not to have to fork the library or switch. We'd love your feedback on the change as soon as possible so we can move forward with a path to get the latest version and not break kubernetes customers.

[niemeyer]

@mikebz My plan/hope was still the one I mentioned above, working on something to fix the issue this week. But I see you both already did some good research on this yourselves meanwhile, and have some PRs that solve the problem for you. So having that on your own branch is not such a bad idea given the time constraints. I had 40+ meetings in the past week, and I'm still hopeful I'll make into my family holidays next week, so while I was attempting to stretch this week, it's not ideal. To be honest indeed I'm much more comfortable with you fixing that in your branch and not blocking on me, while I can look into this issue with proper slice of time.

[natasha41575]

@niemeyer are there any updates? We would love to remove our internal fork as soon as we can.

[mikebz]

@niemeyer we have forked and applied our fix here: https://github.com/kubernetes-sigs/kustomize/tree/master/kyaml/internal/forked/github.com/go-yaml/yaml , but don't want to have a growing delta with your code for too long. Can we help migrate this to mainline or do something else? Thank you!