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

Preserve comments #4353

Closed
stalb opened this issue Dec 27, 2021 · 6 comments
Closed

Preserve comments #4353

stalb opened this issue Dec 27, 2021 · 6 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. triage/under-consideration

Comments

@stalb
Copy link

stalb commented Dec 27, 2021

I know that #261 #259 and similar issues have been closed (because they were out of scope of kustomize).
But since go-yaml/yaml v3 now supports comments decoding and encoding, do you think you could reopen issue #259.

It would allow to use KRM functions like apply-setters and create-setters as transformers which is note possible now because kustomize removes comments before applying transformers as explained in #3953 (comment)

@stalb stalb added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 27, 2021
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Dec 27, 2021
@k8s-ci-robot
Copy link
Contributor

@stalb: This issue is currently awaiting triage.

SIG CLI takes a lead on issue triage for this repo, but any Kubernetes member can accept issues by applying the triage/accepted label.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@natasha41575
Copy link
Contributor

natasha41575 commented Dec 30, 2021

While we would like to and have discussed preserving comments, there are a few things in the way. First, we use sigs.k8s.io/yaml, which depends on yaml.v2. It seems like there is a fair amount of work to be done before it can be upgraded to yaml.v3, here is the PR that attempts to do so.

We also have a direct dependency on yaml.v2. Kustomize uses yaml.v2 for nearly all of its marshaling and unmarshaling, we will have to see how feasible it is to (a) update everything to yaml.v3 and (b) add support for comment preservation on top of that.

The last (and IMO biggest) hurdle is that kustomize is currently maintaining its own internal fork of yaml.v3 (see the history here) - and we want to get rid of this as quickly as possible. It unfortunately seems that the updates we need in yaml.v3 aren't going to make it into the upstream repo anytime soon, so we are stuck maintaining the fork until we come up with an alternative solution.

I am inclined to support prioritizing removing our fork of yaml.v3, before updating all of our kustomize yaml marshaling to depend on it. That unfortunately would mean that comment support would have to wait until we have a better idea of how we want to proceed with yaml.v3.

For the case of setters specifically, you can currently run kustomize fn run to run KRM functions that depend on comments.

/triage under-consideration

@natasha41575 natasha41575 removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Dec 30, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 30, 2022
@stalb
Copy link
Author

stalb commented Mar 31, 2022

I understand why it's not a priority, so I'm closing it.
/close

@k8s-ci-robot
Copy link
Contributor

@stalb: Closing this issue.

In response to this:

I understand why it's not a priority, so I'm closing it.
/close

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.

@eternalphane
Copy link

I am inclined to support prioritizing removing our fork of yaml.v3, before updating all of our kustomize yaml marshaling to depend on it. That unfortunately would mean that comment support would have to wait until we have a better idea of how we want to proceed with yaml.v3.

@natasha41575 Now that the internal fork has been moved to upstream, can you please kindly reopen this issue?

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. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. triage/under-consideration
Projects
None yet
Development

No branches or pull requests

5 participants