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

kpt fn render reformats causes undesired field reordering #2332

Closed
morgante opened this issue Jun 24, 2021 · 9 comments · Fixed by #2376
Closed

kpt fn render reformats causes undesired field reordering #2332

morgante opened this issue Jun 24, 2021 · 9 comments · Fixed by #2376
Assignees
Labels
area/hydrate bug Something isn't working triaged Issue has been triaged by adding an `area/` label
Milestone

Comments

@morgante
Copy link
Contributor

When using kpt fn render, comments in my yaml files are arbitrarily relocated and disconnected from their intended location.

For example, if I run kpt fn render on this ConfigMap:

apiVersion: v1
kind: ConfigMap
metadata:
  name: setters-config
data:
  # This should be the name of your Config Controller instance
  cluster-name: cluster-name
  # This should be the project where you deployed Config Controller
  project-id: project-id
  project-number: "1234567890123"
  # You can leave these defaults
  namespace: config-control
  deployment-repo: deployment-repo
  source-repo: source-repo

It will reorganize the entire file and disconnect comments from their intended location:

apiVersion: v1
kind: ConfigMap
metadata: # kpt-merge: /setters-config
  name: setters-config
data:
  # You can leave these defaults
  namespace: config-control
  # This should be the name of your Config Controller instance
  cluster-name: cluster-name
  deployment-repo: deployment-repo
  # This should be the project where you deployed Config Controller
  project-id: project-id
  project-number: "1234567890123"
  source-repo: source-repo

Desired result: The order of fields in the data map should not be changed and comments should stay connected to the correct lines.

kpt version: v1.0-beta

@morgante morgante added the bug Something isn't working label Jun 24, 2021
@frankfarzan frankfarzan added this to the v1.0 m4 milestone Jun 24, 2021
@frankfarzan frankfarzan added triaged Issue has been triaged by adding an `area/` label area/hydrate labels Jun 24, 2021
@mengqiy
Copy link
Contributor

mengqiy commented Jun 24, 2021

If you look at the yaml lines with comment, the comments are still tied to the same yaml line after the fields reordering.

  # This should be the name of your Config Controller instance
  cluster-name: cluster-name
  # This should be the project where you deployed Config Controller
  project-id: project-id
  # You can leave these defaults
  namespace: config-control

The comments preservation works as expected.
It seems what causes the problem here is field reordering by formatting yaml.

@mengqiy mengqiy changed the title kpt fn render reformats comments into the wrong location. kpt fn render reformats causes undesired field reordering Jun 24, 2021
@morgante
Copy link
Contributor Author

Right, they're still tied to the immediately following line. The problem is the whole map being rearranged.

@phanimarupaka
Copy link
Contributor

Here is how formatting works. https://catalog.kpt.dev/format/v0.1/?id=functionconfig

This falls into the category of unrecognized fields and are being sorted. I will try to figure out a way to handle this, probably not sort the unrecognized fields and retain their original order. Need to see if there is any downside due to that approach.

How big of a problem is it ? If you run render again, the order is not changed correct ? I see the problem with this comment though # You can leave these defaults. You want the order to be preserved for this. Is there a work around?

@morgante
Copy link
Contributor Author

How big of a problem is it ? If you run render again, the order is not changed correct ? I see the problem with this comment though # You can leave these defaults. You want the order to be preserved for this. Is there a work around?

It's not a blocker, but meaningful friction if we're trying to encourage in-place hydration. I'm not aware of any workaround where my goal is preserved (putting the defaultable values at the bottom).

@bzub
Copy link
Contributor

bzub commented Jun 25, 2021

#2301

@stefanhenseler
Copy link

stefanhenseler commented Jul 4, 2021

@phanimarupaka I think this is really something we need to look at. Our users complain about this as well. This behavior is really odd. Kustomizations suddenly look like this:

namespace: helix-pomerium
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
metadata:
  name: kustomization
  annotations:
    config.kubernetes.io/local-config: 'true'
resources:
  - ../upstream

Especially if you have a preexisting base and suddenly, after running the setter function, you got a hundred changes and the comments are all over the place. I agree, it's not as bad if you recreate a package from scratch and the ordering is already there. I think the behavior should be similar to the old kpt set command if that's possible.

@phanimarupaka
Copy link
Contributor

@stefanhenseler This is being actively worked on. Here is the PR for reference. kubernetes-sigs/kustomize#4021

We will include it in the next kpt release.

@stefanhenseler
Copy link

Ah ok thanks, I think this will solve it for us.

@phanimarupaka
Copy link
Contributor

The PR to fix this issue has been merged and the fix will be available in beta.2 release of kpt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/hydrate bug Something isn't working triaged Issue has been triaged by adding an `area/` label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants