-
Notifications
You must be signed in to change notification settings - Fork 2.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
start running make test on all Kubernetes PRs, drop caching to match bazel test … #21008
Conversation
@@ -10,22 +10,15 @@ presubmits: | |||
labels: | |||
preset-service-account: "true" | |||
optional: true | |||
always_run: false | |||
always_run: true |
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.
I've been testing this, it behaves as well as the bazel job.
We can start running it everywhere but not require it, then transition which job is required, then phase out the old job.
args: | ||
- -c | ||
- | | ||
# Restore the cache |
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.
this is not helping us currently, prefer to match bazel for now (-race)
# Run tests as usual | ||
time make test KUBE_TIMEOUT=--timeout=240s | ||
# TODO: stop depending on custom timeout | ||
- make test KUBE_RACE=-race KUBE_TIMEOUT=--timeout=240s |
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.
we should reduce this down to make test KUBE_RACE=-race
but that should come later, priority is smooth transition
/retest |
38fa668
to
8e7cf3a
Compare
8e7cf3a
to
aadbbc7
Compare
gubernator/config.yaml
Outdated
@@ -34,6 +34,7 @@ jobs: | |||
- pull-kubernetes-e2e-kind | |||
- pull-kubernetes-e2e-kind-ipv6 | |||
- pull-kubernetes-integration | |||
- pull-kubernetes-make-test |
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.
Silly nit I didn't catch before the PR introducing this job merged: WDYT about pull-kubernetes-test
or pull-kubernetes-unit
instead? Avoid baking the name of how they're invoked into the job
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.
+1 for pull-kubernetes-unit
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.
Sure
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
aadbbc7
to
2d11ccf
Compare
2d11ccf
to
67a2283
Compare
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.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, spiffxp 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 |
@BenTheElder: Updated the
|
…-race
/cc @spiffxp @dims @liggitt