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.

Sign up for GitHub

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

Jump to bottom

Add scheduling op to helm chart #697

Merged
merged 1 commit into from
Apr 10, 2024
Merged

Add scheduling op to helm chart #697

merged 1 commit into from
Apr 10, 2024

Conversation

mochizuki875
Copy link
Member

@mochizuki875 mochizuki875 commented Jan 26, 2024 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add ability to change nodeSelector, affinity and tolerations of Pods installed via Helm Chart.

Which issue(s) this PR fixes:

Fixes #696

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Adds ability to change NodeSelector, affinity, and tolerance to Helm Chart.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 26, 2024
@netlify Netlify
Copy link

netlify bot commented Jan 26, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-scheduler-plugins canceled.

Name Link
🔨 Latest commit 70c77ce
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-scheduler-plugins/deploys/661628277cf9d1000848423a

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 26, 2024
@mochizuki875 mochizuki875 force-pushed the add_scheduling_op_helm branch from 7212a7f to 474b8cb Compare January 26, 2024 06:55
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 26, 2024
PiotrProkop
PiotrProkop reviewed Feb 16, 2024
View reviewed changes
Copy link
Contributor

@PiotrProkop PiotrProkop left a comment

Choose a reason for hiding this comment

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

just a small nit, overall looks good to me.

manifests/install/charts/as-a-second-scheduler/templates/deployment.yaml Outdated Show resolved Hide resolved
@mochizuki875 mochizuki875 force-pushed the add_scheduling_op_helm branch from 474b8cb to 54876a2 Compare February 16, 2024 14:30
@PiotrProkop
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 Feb 16, 2024
@mochizuki875
Copy link
Member Author

/assign @Huang-Wei

@Huang-Wei
Copy link
Contributor

Could you add a release note in the PR's description? (in the release-notes section)

/lgtm
/approve

/hold
for release-notes

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei, mochizuki875

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 4, 2024
@mochizuki875
Copy link
Member Author

@Huang-Wei
Thanks, and I've done.
Could you check that?

@mochizuki875
Copy link
Member Author

PTAL?

@Huang-Wei
Copy link
Contributor

PTAL?

ah I suppose you'd have cancelled the /hold youself.

/unhold

@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 Apr 9, 2024
@Huang-Wei
Copy link
Contributor

/retest

@Huang-Wei
Copy link
Contributor

@mochizuki875 could you rebase the PR? (although there is no conflict)

The CI failure is related setup-envtest, which should go away after rebasing.

@mochizuki875 mochizuki875 force-pushed the add_scheduling_op_helm branch from 54876a2 to a8e31a2 Compare April 10, 2024 01:14
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2024
@mochizuki875 mochizuki875 force-pushed the add_scheduling_op_helm branch from a8e31a2 to e68bde0 Compare April 10, 2024 02:38
@mochizuki875
Copy link
Member Author

@Huang-Wei

I've attempted to rebase, but CI failure was not solved.
I've fix setup-envtest version in hack/install-envtest.sh as I think it related to the issue.

FYI: It seems that the same workaround have been taken for #708 that has not been merged yet.

@Huang-Wei
Copy link
Contributor

I've fix setup-envtest version in hack/install-envtest.sh as I think it related to kubernetes-sigs/controller-runtime#2720.

You're right, I had a bad memory... I encountered this issue in my another project and commented here. I thought I fixed in this repo but I didn't...

Thanks a lot for the fix!

Huang-Wei
Huang-Wei reviewed Apr 10, 2024
View reviewed changes
hack/install-envtest.sh Outdated
@@ -32,6 +32,6 @@ version=$(cat ${SCRIPT_ROOT}/go.mod | grep 'k8s.io/kubernetes' | grep -v '=>' |

GOPATH=$(go env GOPATH)
TEMP_DIR=${TMPDIR-/tmp}
go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest
go install sigs.k8s.io/controller-runtime/tools/setup-envtest@release-0.17
Copy link
Contributor

Choose a reason for hiding this comment

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

could you change to 0.15 to align with go.mod:

scheduler-plugins/go.mod

Line 31 in cd3e4fb

sigs.k8s.io/controller-runtime v0.15.0

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I've fixed it.

@mochizuki875 mochizuki875 force-pushed the add_scheduling_op_helm branch from e68bde0 to 70c77ce Compare April 10, 2024 05:48
@Huang-Wei
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 Apr 10, 2024
@k8s-ci-robot k8s-ci-robot merged commit bbd3027 into kubernetes-sigs:master Apr 10, 2024
10 checks passed
@mochizuki875 mochizuki875 mentioned this pull request Apr 10, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PiotrProkop PiotrProkop PiotrProkop left review comments

@Huang-Wei Huang-Wei Huang-Wei left review comments

@seanmalloy seanmalloy Awaiting requested review from seanmalloy

@swatisehgal swatisehgal Awaiting requested review from swatisehgal

Assignees

@Huang-Wei Huang-Wei

@PiotrProkop PiotrProkop

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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ability to add tolerations and nodeSelector on the scheduler-pugins helm chart
4 participants
@mochizuki875 @PiotrProkop @Huang-Wei @k8s-ci-robot