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

feat: check if the request is a dry-run to match CAPI topology design #277

Merged
merged 4 commits into from
Mar 6, 2024
Merged

feat: check if the request is a dry-run to match CAPI topology design #277

merged 4 commits into from
Mar 6, 2024

Conversation

aamoyel
Copy link
Contributor

@aamoyel aamoyel commented Mar 5, 2024

Fix #264

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Mar 5, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 5, 2024
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 5, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 5, 2024
nunnatsa
nunnatsa reviewed Mar 5, 2024
View reviewed changes
pkg/webhookhandler/validator_test.go Outdated
switch operation {
case admissionv1.Create:
req.Object = runtime.RawExtension{
Raw: []byte(runtime.EncodeOrDie(encoder, oldObj)),
Object: oldObj,
}
case admissionv1.Update:
req.DryRun = &defaultReqDryRun
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace with ptr.To(false) and drop defaultReqDryRun

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thx for the tip :)

Copy link
Contributor

@nunnatsa nunnatsa Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It failed to compile in the e2e test: https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubernetes-sigs_cluster-api-provider-kubevirt/277/pull-kubernetes-sigs-cluster-api-provider-kubevirt-e2e/1764970340280176640

please run go mod tidy and push the modified files.

nunnatsa
nunnatsa reviewed Mar 5, 2024
View reviewed changes
Copy link
Contributor

@nunnatsa nunnatsa left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Mar 5, 2024
@coveralls
Copy link

coveralls commented Mar 5, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 8158792860

Details

  • 3 of 5 (60.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 63.346%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/webhookhandler/validator.go 3 5 60.0%
Totals Coverage Status
Change from base Build 7425711507: -0.01%
Covered Lines: 1011
Relevant Lines: 1596
💛 - Coveralls

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 5, 2024
@aamoyel
Copy link
Contributor Author

aamoyel commented Mar 5, 2024

Tests needs to be fixed @nunnatsa ? Or just re request ?

@nunnatsa
Copy link
Contributor

nunnatsa commented Mar 5, 2024 •
edited
Loading

I don't think we need to touch the test. just run go mod tidy it will probably modify go.sum, maybe also go.mod. just push the result.

BTW, we currently can't re-run e2e. the only way is to push again, e.g.

git commit --amend
git push -f

============ EDIT =============
I guess you did run go mod tidy because both go.mod and go.sum seems up to date

Try to re-push then

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 5, 2024
@aamoyel
Copy link
Contributor Author

aamoyel commented Mar 5, 2024

No changes after many e2e restart

@nunnatsa
Copy link
Contributor

nunnatsa commented Mar 5, 2024

No changes after many e2e restart

Found it: you'll need to also run go mod tidy from the e2e directory.

@nunnatsa
Copy link
Contributor

nunnatsa commented Mar 6, 2024

/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 6, 2024
@nunnatsa
Copy link
Contributor

nunnatsa commented Mar 6, 2024

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aamoyel, nunnatsa

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 Mar 6, 2024
@k8s-ci-robot k8s-ci-robot merged commit f80289f into kubernetes-sigs:main Mar 6, 2024
13 of 14 checks passed
@aamoyel aamoyel deleted the topology-dryrun-check branch March 27, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nunnatsa nunnatsa nunnatsa left review comments

@neolit123 neolit123 Awaiting requested review from neolit123

@qinqon qinqon Awaiting requested review from qinqon

Assignees

@nunnatsa nunnatsa

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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ClusterClass incompatibility caused by KubevirtMachineTemplateSpec immutability.
4 participants
@aamoyel @coveralls @nunnatsa @k8s-ci-robot