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
Support "Clone API" to filter virtualmachine.spec.template.annotation… #10658
Conversation
Hi @matthewei. Thanks for your PR. I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
/test |
@matthewei: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
/ok-to-test |
@matthewei: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
/needs-ok-test |
/ok-to-test |
/cc @mhenriks |
/cc @0xFelix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK to me but there are still failing tests. Can you please address those?
@@ -108,11 +114,12 @@ func validateFilters(filters []string, fieldName string) (causes []metav1.Status | |||
return nil | |||
} | |||
|
|||
errPattern := "%s filter %s is invalid: cannot contain a %s character (%s); FilterRules: %s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Keep definition of this at L129 and as const
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/virtctl/create/clone/clone.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put the changes to virtctl into a separate commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, will do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done ,will raise another PR
/retest-required |
f553c48
to
c4339bc
Compare
I will check the e2e code. |
c4339bc
to
2bcf283
Compare
c0700c7
to
500b1f2
Compare
3765ca6
to
321e5b7
Compare
/retest-required |
3c29f05
to
a39f1a5
Compare
… or virtualmachine.spec.template.label Signed-off-by: shenwei <shenwei_yewu@cmss.chinamobile.com>
a39f1a5
to
0e7658d
Compare
/retest-required |
If this PR can be merged and I would like to modify clone_api. |
@0xFelix Hi, I have finished this PR. Please move to next step. Thanks a lot. |
/approve |
/retest-required |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mhenriks 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 |
can I raise a PR for clone_api? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/lgtm
/retest-required
You mean a PR for updating the docs on the clone API? Please go ahead! |
/retest-required |
2 similar comments
/retest-required |
/retest-required |
@matthewei: The following tests failed, say
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. I understand the commands that are listed here. |
/retest-required |
What this PR does / why we need it:
The label--annotation-filters strip it from the top-level just as your description and it can't strip from spec.template.metadata.annotation or labels. Some cni such as kube-ovn will inject network information into the annotation. So we need to filter them out.
please theck the following example.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #10374
Special notes for your reviewer:
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note: