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

🌱 Move webhooks into pkg/webhooks #1920

Merged
merged 3 commits into from
Mar 5, 2024
Merged

🌱 Move webhooks into pkg/webhooks #1920

merged 3 commits into from
Mar 5, 2024

Conversation

mdbooth
Copy link
Contributor

@mdbooth mdbooth commented Mar 4, 2024 •
edited
Loading

Moves webhooks from api to pkg/webhooks making only mechanical code changes except for:

  • the removal of the defaulting webhooks because they were no-ops
  • the addition of a conversion webhook for OpenStackClusterTemplateList because it was missing

The former results in there now being no mutating webhook configured.

/hold

mdbooth added 3 commits March 2, 2024 00:26
@mdbooth
Move webhooks into pkg/webhooks
750b84d 
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.
@mdbooth
Register API versions with test scheme in validation tests
13e95db 
The conversion webhook does not try to register conversions for versions
which are not present in the scheme.
@mdbooth
Implement convertible for OpenStackClusterTemplateList
cb09d5f 
The conversion webhook fails to register without this.
@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 4, 2024
@k8s-ci-robot k8s-ci-robot requested review from dulek and EmilienM March 4, 2024 12:38
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdbooth

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 4, 2024
@netlify Netlify
Copy link

netlify bot commented Mar 4, 2024 •
edited
Loading

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

Name Link
🔨 Latest commit cb09d5f
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65e637e917bdc80008f33a00
😎 Deploy Preview https://deploy-preview-1920--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 site configuration.

@mdbooth mdbooth mentioned this pull request Mar 4, 2024
Merged
@mdbooth
Copy link
Contributor Author

mdbooth commented Mar 4, 2024

Probably not necessary here, but:
/test pull-cluster-api-provider-openstack-e2e-full-test

@mdbooth
Copy link
Contributor Author

mdbooth commented Mar 4, 2024

I think we still need to define webhooks for the list types even if they don't implement a validator or defaulter, because they will implicitly register a conversion webhook.

@mdbooth
Copy link
Contributor Author

mdbooth commented Mar 4, 2024

/test pull-cluster-api-provider-openstack-e2e-full-test

@mdbooth
Copy link
Contributor Author

mdbooth commented Mar 4, 2024 •
edited
Loading

The e2e tests were failing because I had added a conversion webhook for OpenStackClusterTemplateList which was not previously present. I added it because it looked like an accidental omission, but it turns out that:

  • It does not implement Convertible for old versions
  • This caused the e2e tests to fail because the conversion webhook expects all versions of the Hub type which are registered in the current scheme to be convertible, which they weren't.

This also explains why the API validation tests weren't failing, because they weren't registering old API versions in the test scheme. I've fixed this by:

  • Registering the old API versions in the test scheme of the API validations tests
  • Fixed the resulting test failure by implementing Convertible where it was missing.

I've left these things as separate commits because they're logically separate changes. However, they are all required for the tests to pass on this PR.

@mdbooth
Copy link
Contributor Author

mdbooth commented Mar 4, 2024

/test pull-cluster-api-provider-openstack-e2e-full-test

@mdbooth
Copy link
Contributor Author

mdbooth commented Mar 5, 2024

Given that the full test passed, I'm guessing that could have been a flake.

/retest-required

This was referenced Mar 5, 2024
Merged
Merged
@JoelSpeed
Copy link

/lgtm

Big step in the right direction towards decoupling API types from imports, LGTM

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2024
@mdbooth
Copy link
Contributor Author

mdbooth commented Mar 5, 2024

/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 Mar 5, 2024
@k8s-ci-robot k8s-ci-robot merged commit e6bb34f into kubernetes-sigs:main Mar 5, 2024
10 checks passed
@mdbooth mdbooth deleted the webhooks branch March 5, 2024 12:44
@huxcrux huxcrux mentioned this pull request Mar 5, 2024
Closed
@mdbooth mdbooth mentioned this pull request Mar 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dulek dulek Awaiting requested review from dulek

@EmilienM EmilienM Awaiting requested review from EmilienM

Assignees

@JoelSpeed JoelSpeed

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
CAPO Roadmap
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mdbooth @k8s-ci-robot @JoelSpeed