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 legacy resource ordering customizable in the kustomization.yaml file #3913

Closed
yanniszark opened this issue May 21, 2021 · 19 comments · Fixed by #4019
Closed

Make legacy resource ordering customizable in the kustomization.yaml file #3913

yanniszark opened this issue May 21, 2021 · 19 comments · Fixed by #4019
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@yanniszark
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Related issue: #3794

TLDR:

Describe the solution you'd like

Make resource ordering configurable via the kustomization.yaml file.
The sorter accepts the orderFirst and orderLast arguments, to know which resources to order first and last.
Then we need to decide what to call the new field for the sorter in the kustomization.yaml file.

cc @monopole

@monopole monopole added triage/accepted Indicates an issue or PR is ready to be actively worked on. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels May 25, 2021
@KnVerey
Copy link
Contributor

KnVerey commented May 25, 2021

I think that this new field should replace the --reorder flag. Because it changes the output, this flag is currently violating Kustomize's principle of having no build-time side-effects. IMO we should add a deprecation warning to the flag when this new field merges, and then remove the flag from a future major version release. While both are present, the field should take precedence (if you've switched your kustomizations to the new way, we should ignore the old)... though this might be complicated in practice because the flag's scope is global and the field's should not be.

Maybe something like this?

sortOrder: {legacy,fifo} # expandable e.g. if we want to add a non-legacy option that provides semantically stable ordering but without the release order implications
legacySortOptions: #  it's an error if this is present and sortOrder isn't legacy
  orderFirst:
  - {GVK}
  orderLast:
  - {GVK}

Or just the latter field, and stock legacy ordering would be enabled with legacySortOptions: {}. I don't love the clutter of two new top-level fields, but the latter reads awkwardly to me.

@yanniszark wdyt of the above? Are you interested in contributing this field?

@KnVerey KnVerey removed their assignment May 25, 2021
@yanniszark
Copy link
Contributor Author

Hi @KnVerey,
I think the discussion about kustomize removing the reorder flag is very interesting and something we should consider after this feature lands. We (Arrikto, Kubeflow) are interested in pushing this feature forward, so I will start with the example you suggested as my goal.

@KnVerey
Copy link
Contributor

KnVerey commented May 28, 2021

/assign @yanniszark

@numbsafari
Copy link

Brought here by some problems deploying cert-manager together with some other tools using kustomize. I think this idea of having a logical default ordering, but the ability on a per-kustomization basis to describe a desired ordering based on resource type makes a lot of sense. The main reason I like it is that you can memorialize/document the need for the ordering within your data/files, rather than relying on an external side-effect registered via a CLI flag. In addition, given the existence of CustomResourceDefinitions with their own potential semantics, it's impossible for kustomize to know "everything" it needs to know.

yanniszark added a commit to yanniszark/kustomize that referenced this issue Jul 6, 2021
Extend the kustomization.yaml API with the 'sortOrder' and
'legacySortOptions' fields.

Refs kubernetes-sigs#3913

Signed-off-by: Yannis Zarkadas <[email protected]>
yanniszark added a commit to yanniszark/kustomize that referenced this issue Jul 6, 2021
Extend the kustomization.yaml API with the 'sortOrder' and
'legacySortOptions' fields.

Refs kubernetes-sigs#3913

Signed-off-by: Yannis Zarkadas <[email protected]>
@natasha41575
Copy link
Contributor

natasha41575 commented Jul 7, 2021

I don't love the clutter of two new top-level fields, but the latter reads awkwardly to me.

Would it make sense to consolidate it into a single top-level field with nested subfields?

Instead of erroring if legacySortOptions is present and sortOrder isn't legacy, we could choose to make sortOrder and legacySortOptions mutually exclusive; the presence of legacySortOptions could implicitly mean that the sort order should be legacy.

To that end, we could just call it legacy instead of legacySortOptions to be more concise.

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
- ../base

sortOptions:
  legacy:
    orderFirst:
    - {GVK}
    orderLast:
    - {GVK}

vs

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
- ../base

sortOptions:
  sortOrder: fifo

Stock legacy ordering would be enabled with

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
- ../base

sortOptions:
  sortOrder: legacy

@KnVerey @yanniszark WDYT?

@yanniszark
Copy link
Contributor Author

@natasha41575 IIUC, you are proposing this:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
- ../base

sortOptions:
  legacy:
    orderFirst:
    - {GVK}
    orderLast:
    - {GVK}

VS

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
- ../base

sortOptions:
  sortOrder: legacy
  legacySortOptions:
    orderFirst:
    - {GVK}
    orderLast:
    - {GVK}

TBF, the second one seems more intuitive to me. @KnVerey what do you think?

@natasha41575
Copy link
Contributor

natasha41575 commented Jul 13, 2021

@KnVerey please let us know your thoughts

I'm happy with the latter of

sortOptions:
  sortOrder: legacy
  legacySortOptions:
    orderFirst:
    - {GVK}
    orderLast:
    - {GVK}

It was mostly having two new top-level fields that seemed a bit cluttered to me.

@yanniszark
Copy link
Contributor Author

ping @KnVerey
@natasha41575 I will start the implementation of the latter in the PR, while we wait.
Hopefully, there will only be small changes on top of that :)

@KnVerey
Copy link
Contributor

KnVerey commented Jul 21, 2021

That LGTM. I'd suggest dropping sortOrder to just order since it is under sortOptions, but I don't feel strongly about it.

@natasha41575
Copy link
Contributor

natasha41575 commented Jul 21, 2021

I'd suggest dropping sortOrder to just order since it is under sortOptions, but I don't feel strongly about it.

+1

yanniszark added a commit to yanniszark/kustomize that referenced this issue Jul 27, 2021
Make orderFirst and orderLast public in preparation of changes to make
the resource ordering configurable.

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

kustomization: Extend with sortOrder and legacySortOptions fields

Extend the kustomization.yaml API with the 'sortOrder' and
'legacySortOptions' fields.

Refs kubernetes-sigs#3913

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

plugins: Extend LegacyOrderTransformer with customizable ordering

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

plugins: Generate new LegacyOrderTransformer

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

Fix existing LegacyOrderTransformer invocations

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

Throw an error if legacySortOptions is set and sortOrder is not legacy

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

api: Updated dep files after running tests

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

cmd/config: Updated dep files after running tests

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

cmd/pluginator: Updated dep files after running tests

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

kustomize: Updated dep files after running tests

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

wip

Signed-off-by: Yannis Zarkadas <[email protected]>
yanniszark added a commit to yanniszark/kustomize that referenced this issue Jul 27, 2021
Make orderFirst and orderLast public in preparation of changes to make
the resource ordering configurable.

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

kustomization: Extend with sortOrder and legacySortOptions fields

Extend the kustomization.yaml API with the 'sortOrder' and
'legacySortOptions' fields.

Refs kubernetes-sigs#3913

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

plugins: Extend LegacyOrderTransformer with customizable ordering

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

plugins: Generate new LegacyOrderTransformer

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

Fix existing LegacyOrderTransformer invocations

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

Throw an error if legacySortOptions is set and sortOrder is not legacy

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

api: Updated dep files after running tests

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

cmd/config: Updated dep files after running tests

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

cmd/pluginator: Updated dep files after running tests

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

kustomize: Updated dep files after running tests

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

wip

Signed-off-by: Yannis Zarkadas <[email protected]>
@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 Oct 19, 2021
@natasha41575
Copy link
Contributor

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 19, 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 Jan 17, 2022
@KnVerey
Copy link
Contributor

KnVerey commented Jan 17, 2022

#4019 is back in active development and getting really close now

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 17, 2022
@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 Apr 17, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active 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 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 rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 17, 2022
@bjw-s
Copy link

bjw-s commented May 24, 2022

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label May 24, 2022
@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 Aug 22, 2022
@onedr0p
Copy link

onedr0p commented Aug 22, 2022

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 22, 2022
yanniszark added a commit to yanniszark/cli-experimental that referenced this issue Dec 9, 2022
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]>
yanniszark added a commit to yanniszark/cli-experimental that referenced this issue Dec 9, 2022
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]>
yanniszark added a commit to yanniszark/cli-experimental that referenced this issue Jan 11, 2023
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]>
yanniszark added a commit to yanniszark/cli-experimental that referenced this issue Jan 11, 2023
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]>
yanniszark added a commit to yanniszark/cli-experimental that referenced this issue Jan 11, 2023
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. 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.

9 participants