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

[charts/occm] Implement imagePullSecret support for release 1.28 #2445

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

carlotardl
Copy link
Contributor

What this PR does / why we need it:
This PR implements imagePullSecrets for the openstack-cloud-controller-manager helm chart on release 1.28 branch.

Which issue this PR fixes(if applicable):
None applicable

Special notes for reviewers:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. 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 Oct 20, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @carlotardl. Thanks for your PR.

I'm waiting for a kubernetes 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.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 20, 2023
@jichenjc
Copy link
Contributor

/ok-to-test

@k8s-ci-robot 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 Oct 26, 2023
@jichenjc
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2023
@wwentland
Copy link
Contributor

Thank you!

This looks good to me, but the PR needs to be rebased against the release-1.28 branch. It would also be fantastic if you could bump the chart version again and squash the commits in here.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 30, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2023
@carlotardl carlotardl force-pushed the release-1.28 branch 3 times, most recently from 74f0e30 to adcb494 Compare October 30, 2023 09:45
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 30, 2023
@wwentland
Copy link
Contributor

This change looks good, but I am wondering if we should get it into master first before backporting it to release-1.28 ?

Specifically, I would have expected #2446 to look very similar to this set of changes, whereas it appears to implement vastly different ones.

@jichenjc
Copy link
Contributor

good point, let's wait for master merge first

@wwentland
Copy link
Contributor

In fact, we should probably close this and backport the cherry-pick of #2446 that lands in master.

@dulek
Copy link
Contributor

dulek commented Oct 30, 2023

Yup! @carlotardl, can you follow the basic backporting process? We expect the fixes to get into master first and then to the 1.28, 1.27 and 1.26 in that order. After master PR merges you need to comment with /cherry-pick release-1.28 to generate the first backport.

/hold

@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 Oct 30, 2023
@carlotardl
Copy link
Contributor Author

Hi, the changes for the master branch were merged. I was wondering since these are the same changes, that this PR could also be merged and keep the chart version. Thank you.

@wwentland
Copy link
Contributor

Hi, the changes for the master branch were merged. I was wondering since these are the same changes, that this PR could also be merged and keep the chart version. Thank you.

Thank you for getting those into master! I am not entirely sure about backporting them though as they very much constitute a new feature. What is the best way to go about this, @dulek?

That being said, I'm sure that this feature would be very welcomed by users who have to use imagePullSecrets.

Once decided, we'd use prow's cherry-pick functionality to automatically create the backport:

@dulek
Copy link
Contributor

dulek commented Nov 8, 2023

@carlotardl, @wwentland: I think the rule is to always bump the chart version, so it's good this PR does so. I think I'm okay with backporting this, it's kind of a feature, but just configuration feature.

That being said this patch is different than what merged on master. Please follow the regular way and just do a cherry-pick from master.

@wwentland
Copy link
Contributor

Yeah, the automatic backport failed as the chart version has to be bumped to a different version. It would probably be best to cherry pick 95a3823 from master and use a suitable chart version.

Could you be so kind, @carlotardl? I'm also happy to help if required.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 9, 2023
@carlotardl
Copy link
Contributor Author

I cherry-picked the commit 95a382323f6beb58953c9eac6864446ed36b2285 and bumped the version in 2 commits, if they need to be squashed let me know, thank you

@dulek
Copy link
Contributor

dulek commented Nov 9, 2023

I cherry-picked the commit 95a382323f6beb58953c9eac6864446ed36b2285 and bumped the version in 2 commits, if they need to be squashed let me know, thank you

Yup, let's squash it and we're good to go!

@carlotardl
Copy link
Contributor Author

carlotardl commented Nov 14, 2023

Hi, the commits were squashed, could you please take a look? Thank you

@wwentland
Copy link
Contributor

Thank you!

/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 14, 2023
@dulek
Copy link
Contributor

dulek commented Nov 14, 2023

/hold cancel

@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 Nov 14, 2023
@k8s-ci-robot k8s-ci-robot merged commit 2c5cb25 into kubernetes:release-1.28 Nov 14, 2023
1 check passed
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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants