Make ordering configurable #4019

yanniszark · 2021-06-30T16:45:05Z

Resolves #3913

Make kustomize's ordering of resources customizable from the kustomization.yaml file.

My changes in a nutshell:

Add the fields to the kustomization struct
Extend the LegacyResourceOrderingPlugin to be configurable in its ordering.
Wire it up with the new fields.

We may also want to delete the now unused IdSlice code, but I've leaven it up to the reviewer to guide me on this.

cc @KnVerey @monopole

ALLOW_MODULE_SPAN

k8s-ci-robot · 2021-06-30T16:45:07Z

@yanniszark: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

k8s-ci-robot · 2021-06-30T16:45:14Z

Hi @yanniszark. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 · 2021-06-30T17:54:16Z

/ok-to-test

natasha41575 · 2021-06-30T17:56:12Z

As a note, you can run the command make prow-presubmit-check to run all the tests here locally.

yanniszark · 2021-07-01T20:29:02Z

/retest

natasha41575 · 2021-07-01T20:53:30Z

/retest

natasha41575 · 2021-07-02T01:50:04Z

I might have to unpin some modules for the presubmit to pass. I'll try to make some time to do that tomorrow!

yanniszark · 2021-07-06T13:53:35Z

@natasha41575 I'm a bit perplexed by what happened.
The presubmit in the PR fails with:

go: sigs.k8s.io/kustomize/plugin/builtin/legacyordertransformer: package sigs.k8s.io/kustomize/kyaml/resid imported from implicitly required module; to add missing requirements, run:
	go get sigs.k8s.io/kustomize/kyaml/[email protected]
make: *** [Makefile:183: api/builtins/LegacyOrderTransformer.go] Error 1

I pinned it down to the unit tests of the legacy order transformer.
However, this made little sense to me, because that particular module (kyaml) is replaced with the local version.
Perhaps you have more insight on this?

yanniszark · 2021-07-06T13:55:17Z

BTW, I added a test and fixed the behavior of the plugin to follow these rules:

If there is no legacySortOptions field set, use the existing legacy sort options.
If there is a legacySortOptions field present, use its options. orderFirst and orderLast are empty by default.

Test failed because I needed to split changes spanning different modules into separate commits.
Hopefully tests will pass now.

natasha41575 · 2021-07-07T16:31:32Z

I pinned it down to the unit tests of the legacy order transformer.
However, this made little sense to me, because that particular module (kyaml) is replaced with the local version.
Perhaps you have more insight on this?

We ran into this problem too but didn't know why :) but as long as it works now that's fine. I'm running the workflows now, I'll take a closer look at this PR this week.

natasha41575 · 2021-07-07T21:25:27Z

Could you add an e2e test in api/krusty (feel free to create a new file for it, and use the other files in that folder as examples) to demonstrate how one would use this in a kustomization file?

natasha41575

EDIT: I need to read through the issue more carefully actually, you can disregard this comment.

I don't know if having two separate top-level fields is necessary, so I commented on the issue.

Signed-off-by: Yannis Zarkadas <[email protected]>

yanniszark · 2022-12-02T21:11:26Z

@KnVerey done! Let's see if it passes now :)

KnVerey · 2022-12-02T21:59:13Z

/lgtm
/approve

k8s-ci-robot · 2022-12-02T21:59:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KnVerey, yanniszark

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [KnVerey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yanniszark · 2022-12-02T22:40:30Z

@natasha41575 @KnVerey many thanks for all the help in getting this change in :)

KnVerey · 2022-12-02T23:14:26Z

Thank YOU for all your hard work on this feature!

natasha41575 · 2022-12-02T23:17:54Z

I am so happy to see this in! Amazing work and thank you, both of you for seeing this through!

* api: Add new types for customizeable resource ordering Signed-off-by: Yannis Zarkadas <[email protected]> * plugins: Implement SortOrderTransformer plugin Implement the SortOrderTransformer plugin. This plugin allows the user to customize the order that kustomize will output resources in. The API for the plugin is the following: sortOptions: order: legacy | fifo legacySortOptions: orderFirst: - {GVK} orderLast: - {GVK} Signed-off-by: Yannis Zarkadas <[email protected]> * plugins: Add boilerplate and generate code for new SortOrderTransformer Signed-off-by: Yannis Zarkadas <[email protected]> * build: Add option to denote if the reorder flag was set by the user We want to take different actions if the reorder flag was set by the user or filled by the default value. Thus, we propagate this information from build to the krusty options. Signed-off-by: Yannis Zarkadas <[email protected]> * api/krusty: Ensure sort ordering works with CLI flag and kustomization Sort order can be defined in two places: - (new) kustomization file - (old) CLI flag We want the kustomization file to take precedence over the CLI flag. Eventually, we may want to move away from having a CLI flag altogether: kubernetes-sigs#3947 Case 1: Sort order set in kustomization file AND in CLI flag. Print a warning and let the kustomization file take precedence. Case 2: Sort order set in CLI flag only or not at all. Follow the CLI flag (defaults to legacy) and reorder at the end. Case 3: Sort order set in kustomization file only. Simply build the kustomization. Signed-off-by: Yannis Zarkadas <[email protected]> * krusty: Add e2e test for SortOrderTransformer Signed-off-by: Yannis Zarkadas <[email protected]> * plugins: Purge LegacyOrderTransformer Signed-off-by: Yannis Zarkadas <[email protected]> * Update go.work.sum Signed-off-by: Yannis Zarkadas <[email protected]> * review: Make review changes Signed-off-by: Yannis Zarkadas <[email protected]> Signed-off-by: Yannis Zarkadas <[email protected]> Signed-off-by: Yannis Zarkadas <[email protected]>

natasha41575 · 2022-12-09T17:02:19Z

@yanniszark Do you have time to write up some docs before this feature is released next Friday 12/16? The docs live here: https://github.com/kubernetes-sigs/cli-experimental/tree/master/site/content/en/references/kustomize/kustomization

If you are unable to, just let us know and we can try to write something up ourselves.

yanniszark · 2022-12-09T18:56:51Z

@natasha41575 yes I will make sure to do so! I will try to parse the current structure and infer the necessary docs to write, but please feel free to tell me what you would expect as a good set of documentation for this feature.

Kustomize has a new field called "sortOptions" to sort resources: kubernetes-sigs/kustomize#3913 kubernetes-sigs/kustomize#4019 Document what it does and how to use it. Signed-off-by: Yannis Zarkadas <[email protected]>

* api: Add new types for customizeable resource ordering Signed-off-by: Yannis Zarkadas <[email protected]> * plugins: Implement SortOrderTransformer plugin Implement the SortOrderTransformer plugin. This plugin allows the user to customize the order that kustomize will output resources in. The API for the plugin is the following: sortOptions: order: legacy | fifo legacySortOptions: orderFirst: - {GVK} orderLast: - {GVK} Signed-off-by: Yannis Zarkadas <[email protected]> * plugins: Add boilerplate and generate code for new SortOrderTransformer Signed-off-by: Yannis Zarkadas <[email protected]> * build: Add option to denote if the reorder flag was set by the user We want to take different actions if the reorder flag was set by the user or filled by the default value. Thus, we propagate this information from build to the krusty options. Signed-off-by: Yannis Zarkadas <[email protected]> * api/krusty: Ensure sort ordering works with CLI flag and kustomization Sort order can be defined in two places: - (new) kustomization file - (old) CLI flag We want the kustomization file to take precedence over the CLI flag. Eventually, we may want to move away from having a CLI flag altogether: kubernetes-sigs#3947 Case 1: Sort order set in kustomization file AND in CLI flag. Print a warning and let the kustomization file take precedence. Case 2: Sort order set in CLI flag only or not at all. Follow the CLI flag (defaults to legacy) and reorder at the end. Case 3: Sort order set in kustomization file only. Simply build the kustomization. Signed-off-by: Yannis Zarkadas <[email protected]> * krusty: Add e2e test for SortOrderTransformer Signed-off-by: Yannis Zarkadas <[email protected]> * plugins: Purge LegacyOrderTransformer Signed-off-by: Yannis Zarkadas <[email protected]> * Update go.work.sum Signed-off-by: Yannis Zarkadas <[email protected]> * review: Make review changes Signed-off-by: Yannis Zarkadas <[email protected]> Signed-off-by: Yannis Zarkadas <[email protected]> Signed-off-by: Yannis Zarkadas <[email protected]>

dstraub · 2023-03-09T15:40:06Z

In generell, we like this approach.
But how could we use 'fifo' with resources and generators ?
The resources are orderd, but configmaps/secrets created by generators appears at the end.

We have only one exception the the 'legacy' order: Node isn't in the legacy list and comes at least.
But we have to adjust node labels before everything else.

lindhe · 2023-07-17T13:00:06Z

It says in the documentation:

This field is the endorsed way to sort resources. It should be used instead of the --reorder CLI flag, which is deprecated.

It is a bit unclear to me if that just means that it is the endorsed way to set the ordering options (instead of the --reorder flag), or if it is the endorsed way of sorting (i.e. one should set sortOptions.order=fifo to use the new recommended way of working).

Can someone please clarify this to me?

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 30, 2021

k8s-ci-robot requested review from justinsb and pwittrock June 30, 2021 16:45

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 30, 2021

yanniszark force-pushed the feature-configurable-ordering branch from e2feb0f to 324895e Compare June 30, 2021 17:05

natasha41575 added the tide/merge-method-merge Denotes a PR that should use a standard merge by tide when it merges. label Jun 30, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 30, 2021

natasha41575 added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed tide/merge-method-merge Denotes a PR that should use a standard merge by tide when it merges. labels Jun 30, 2021

yanniszark force-pushed the feature-configurable-ordering branch 3 times, most recently from 23d16df to 431b40e Compare July 6, 2021 13:53

natasha41575 reviewed Jul 7, 2021

View reviewed changes

yanniszark force-pushed the feature-configurable-ordering branch from 431b40e to 370634b Compare July 27, 2021 21:29

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 27, 2021

yanniszark force-pushed the feature-configurable-ordering branch from 370634b to bb9e19d Compare July 27, 2021 22:33

yanniszark and others added 4 commits December 2, 2022 16:10

krusty: Add e2e test for SortOrderTransformer

202768f

Signed-off-by: Yannis Zarkadas <[email protected]>

plugins: Purge LegacyOrderTransformer

a05b8fb

Signed-off-by: Yannis Zarkadas <[email protected]>

Update go.work.sum

4e0cb79

Signed-off-by: Yannis Zarkadas <[email protected]>

review: Make review changes

e581778

Signed-off-by: Yannis Zarkadas <[email protected]>

yanniszark force-pushed the feature-configurable-ordering branch from 8f14277 to e581778 Compare December 2, 2022 21:11

k8s-ci-robot assigned KnVerey Dec 2, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 2, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2022

k8s-ci-robot merged commit a502717 into kubernetes-sigs:master Dec 2, 2022

yanniszark mentioned this pull request Dec 9, 2022

Document sortOptions field kubernetes-sigs/cli-experimental#306

Merged

cailyn-codes mentioned this pull request Jan 9, 2023

[WIP]: Deprecate Reorder Flag in Favour of Field #4966

Closed

KnVerey mentioned this pull request Jan 9, 2023

Deprecate the reorder flag in favour of a Kustomization field #3947

Closed

kimwnasptd mentioned this pull request Jan 25, 2023

Manual installation instructions and Kustomize 4 kubeflow/manifests#2360

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ordering configurable #4019

Make ordering configurable #4019

yanniszark commented Jun 30, 2021 •

edited by natasha41575

Loading

k8s-ci-robot commented Jun 30, 2021

k8s-ci-robot commented Jun 30, 2021

natasha41575 commented Jun 30, 2021

natasha41575 commented Jun 30, 2021

yanniszark commented Jul 1, 2021

natasha41575 commented Jul 1, 2021

natasha41575 commented Jul 2, 2021

yanniszark commented Jul 6, 2021

yanniszark commented Jul 6, 2021 •

edited

Loading

natasha41575 commented Jul 7, 2021 •

edited

Loading

natasha41575 commented Jul 7, 2021

natasha41575 left a comment •

edited

Loading

yanniszark commented Dec 2, 2022

KnVerey commented Dec 2, 2022

k8s-ci-robot commented Dec 2, 2022

yanniszark commented Dec 2, 2022

KnVerey commented Dec 2, 2022

natasha41575 commented Dec 2, 2022

natasha41575 commented Dec 9, 2022

yanniszark commented Dec 9, 2022

dstraub commented Mar 9, 2023 •

edited

Loading

lindhe commented Jul 17, 2023

Make ordering configurable #4019

Make ordering configurable #4019

Conversation

yanniszark commented Jun 30, 2021 • edited by natasha41575 Loading

k8s-ci-robot commented Jun 30, 2021

k8s-ci-robot commented Jun 30, 2021

natasha41575 commented Jun 30, 2021

natasha41575 commented Jun 30, 2021

yanniszark commented Jul 1, 2021

natasha41575 commented Jul 1, 2021

natasha41575 commented Jul 2, 2021

yanniszark commented Jul 6, 2021

yanniszark commented Jul 6, 2021 • edited Loading

natasha41575 commented Jul 7, 2021 • edited Loading

natasha41575 commented Jul 7, 2021

natasha41575 left a comment • edited Loading

Choose a reason for hiding this comment

yanniszark commented Dec 2, 2022

KnVerey commented Dec 2, 2022

k8s-ci-robot commented Dec 2, 2022

yanniszark commented Dec 2, 2022

KnVerey commented Dec 2, 2022

natasha41575 commented Dec 2, 2022

natasha41575 commented Dec 9, 2022

yanniszark commented Dec 9, 2022

dstraub commented Mar 9, 2023 • edited Loading

lindhe commented Jul 17, 2023

yanniszark commented Jun 30, 2021 •

edited by natasha41575

Loading

yanniszark commented Jul 6, 2021 •

edited

Loading

natasha41575 commented Jul 7, 2021 •

edited

Loading

natasha41575 left a comment •

edited

Loading

dstraub commented Mar 9, 2023 •

edited

Loading