-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
integration test for cronjob with controllerv2 #104570
Conversation
Hi @RyanAoh. 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 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. |
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.
Try asking for a reviewer in the #sig-apps channel of k8s slack.
@@ -0,0 +1,80 @@ | |||
/* | |||
Copyright 2018 The Kubernetes Authors. |
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.
New files should be current year
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.
Fixed. PTAL.
Thanks.
/sig apps |
7f3c26a
to
1277653
Compare
ideally this should be reviewed by the relevant code / package owners. https://github.com/kubernetes/kubernetes/tree/12776530b21fc73b4944b963c00497acc0feeab9/test/integration/cronjob doesn't have an OWNERS file, but CronJobs are owned by SIG Apps. |
Thanks @neolit123 . I've sent a message in the #sig-apps channel of k8s slack for asking someone to review this. |
/cc @alaypatel07 |
/cc @soltysh |
/ok-to-test |
The test code looks fine, but Does the cronjob integration test code still need to keep the old controller? Can we just switch parts of the integration test code from the old controller to the new CronJobControllerV2 controller? @soltysh |
Yeah, it doesn't make sense to add additional controller. We rather need to switch the existing test to use the new controller instead. |
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.
/assign
Updated. I switched the existing test to use the new controller. |
/retest |
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.
/lgtm
/approve
/triage accept |
@soltysh: The label(s) 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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RyanAoh, soltysh 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 |
/triage accepted |
What type of PR is this?
/kind flake
What this PR does / why we need it:
CronJobControllerV2 is GA in 1.22. But There are no integration tests for it.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: