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

Start using helpers from k8s.io/component-helpers #428

Merged

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Oct 13, 2020

With the new k8s.io/component-helpers repo, we can start replacing many of the helpers that were duplicated in 431597d with their new external equivalents.

This function will depend on the PR kubernetes/kubernetes#95531 to make it available in the new repo

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 13, 2020
@damemi
Copy link
Contributor Author

damemi commented Oct 13, 2020

/hold
for upstream PR

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 13, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2020
@damemi damemi force-pushed the component-helper-nodeselector branch from 5454542 to cc906ee Compare October 30, 2020 13:32
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2020
Copy link
Contributor Author

@damemi damemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold cancel
/cc @seanmalloy @ingvagabund
This sets us up to start importing more helpers and remove dupe code 🎉

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 30, 2020
@damemi damemi force-pushed the component-helper-nodeselector branch from cc906ee to b27dc5f Compare October 30, 2020 13:41
Copy link
Contributor

@ingvagabund ingvagabund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Are there any stability guarantees of the imported helper function? Stronly enforced during PR reviews
  2. Any blockers for importing k8s.io/component-helpers in the descheduler repository? The repository is a staging repository, thus pulling only other staging repositories, not bringing anything from outside. The first release is aligned with 1.20 at which point descheduler will no longer rely on a non-release revision. In order to provide k8s stability guarantees of the descheduler itself, all k8s dependency versions need to align with the same k8s release. The next descheduler release is planned for 1.20 at which point all k8s dependencies (including k8s.io/component-helpers) will get pinned to 1.20.
  3. How using of k8s.io/component-helpers repo is going to affect this repository? Removing and avoiding duplicates

In future, the descheduler will depend more on k8s.io/component-helpers once the scheduling framework is fully migrated under k8s.io/kube-scheduler. This is a necessary prerequisite to achieving that goal.

k8s.io/code-generator v0.19.0
k8s.io/component-base v0.19.2
k8s.io/component-helpers v0.20.0-alpha.2.0.20201030045251-0fa5e458da80
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until there's the first release of k8s.io/component-helpers, there are no stability guarantees of the API in the right sense even though the stability intention is strongly enforced in k8s.io/component-helpers PR reviews. Given the descheduler is the first repo outside of kubernetes org to import the new staging repo and for the stability assumption mentioned above, we can use the descheduler repo as a guinea pig until the first release is out.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, ingvagabund

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:

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

@ingvagabund
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2020
@k8s-ci-robot k8s-ci-robot merged commit b30bd40 into kubernetes-sigs:master Nov 2, 2020
briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022
…odeselector

Start using helpers from k8s.io/component-helpers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants