Skip to content

[release-0.9] 🌱 Move webhooks into pkg/webhooks #2612

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

Open
wants to merge 1 commit into
base: release-0.9
Choose a base branch
from

Conversation

stephenfin
Copy link
Contributor

This is a manual backport of #1920.

/hold

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 26, 2025
Copy link

netlify bot commented Jun 26, 2025

Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
🔨 Latest commit d645938
🔍 Latest deploy log https://app.netlify.com/projects/kubernetes-sigs-cluster-api-openstack/deploys/685d7cc39cf9d7000895496c
😎 Deploy Preview https://deploy-preview-2612--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 26, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign emilienm for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 requested review from EmilienM and mdbooth June 26, 2025 11:23
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 26, 2025
@stephenfin
Copy link
Contributor Author

fwiw, this backport should allow us to address an issue that is currently blocking work on the migration path from MAPO to CAPO in OpenShift in OpenShift. While I appreciate this is primarily for downstream, I'm proposing it upstream as I hope it should introduce minimal risk while allowing us/me to stay as close to upstream as possible (always my preferred approach, personally). I'm happy to discuss why we need this (tl;dr: MAPO is stuck with CAPO v0.9.x for $reasons) and build a case for doing this here, as needed, though if we really don't want to do this here I'd understand.

@stephenfin stephenfin marked this pull request as ready for review June 26, 2025 14:39
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 26, 2025
@k8s-ci-robot k8s-ci-robot requested a review from jichenjc June 26, 2025 14:40
@stephenfin
Copy link
Contributor Author

(marking as ready-for-review with a hold to get CI results)

@mnaser
Copy link
Contributor

mnaser commented Jun 26, 2025

anything that helps an active downstream get closer to upstream is something I am in favour of :)

@stephenfin stephenfin force-pushed the webhooks-0.9 branch 2 times, most recently from bb45ad9 to 6e5f0d9 Compare June 26, 2025 15:39
Moves webhooks from api to pkg/webhooks making only mechanical code
changes except for the removal of the defaulting webhooks, because they
weren't used. This results in there now being no mutating webhook
configured.

NOTE(stephenfin): There were a lot of conflicts here. These were mostly
mitigated by faking the addition of v1alpha8, which moved the webhooks
to the 'api/v1alpha8' package (commit 750b84d), followed by the
subsequent rename of this package to v1beta1 (commit e9fb53c), for
the webhook files and tests. This still resulted in some merge conflicts
due the v1alpha8 changes such as 564b6bd and 4368c4f (which we
obviously don't want to include here) but it made the backport much
simpler.

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

(cherry picked from commit 750b84d)
@k8s-ci-robot
Copy link
Contributor

@stephenfin: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-provider-openstack-test d645938 link true /test pull-cluster-api-provider-openstack-test
pull-cluster-api-provider-openstack-e2e-test d645938 link true /test pull-cluster-api-provider-openstack-e2e-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

@mandre
Copy link
Contributor

mandre commented Jul 14, 2025

I looks like the problems with the CI are due to a mismatch of versions between go and golangci-lint. By the look of https://storage.googleapis.com/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_cluster-a[…]-provider-openstack-test/1938281313530286080/prowjob.json it seems the job uses gcr.io/k8s-staging-test-infra/kubekins-e2e:v20250613-876fb90a97-master with go 1.24.

But golangci-lint is at version v1.54.2 which is not compatible with go 1.24. I wonder if bumping it to 1.64 (first version to support go 1.24) helps.

@mandre
Copy link
Contributor

mandre commented Jul 14, 2025

But golangci-lint is at version v1.54.2 which is not compatible with go 1.24. I wonder if bumping it to 1.64 (first version to support go 1.24) helps.

I tried bumping golangci-lint with #2619 and it's now effectively showing different errors, with gosec and revive linters, while we previously had typecheck errors due to version incompatibilities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Status: Inbox
Development

Successfully merging this pull request may close these issues.

4 participants