Add presubmit job to kubernetes-sigs/node-readiness-controller#37035
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
sig/testing
k8s-ci-robot
added
the
cncf-cla: yes
Priyankasaggu11929
left a comment
Priyankasaggu11929
left a comment
There was a problem hiding this comment.
Thank you for the PR, @vitorfloriano.
Just left one inline comment, but otherwise it looks good to me. Thanks!
| always_run: true | ||
| skip_if_only_changed: "^docs/|\\.md$|^(README|LICENSE|OWNERS|SECURITY_CONTACTS)$" | ||
| branches: | ||
| - ^main$ |
There was a problem hiding this comment.
let's also run the job for release-v* branches
| command: | ||
| - make | ||
| args: | ||
| - test |
There was a problem hiding this comment.
Just one question: make test generates a cover.out artifact. Should I do something with it? Or is it picked-up somehow by the TestGrid?
There was a problem hiding this comment.
setting "decorate: true" inserts an env variable to the prow job container - $ARTIFACTS (usually pointing to /logs/artifacts/ path in the container)
so for cover.out or any other artifacts to show up in the Prow spyglass (prow.k8s.io) artifacts tab, copy/move these files to $ARTIFACTS directory path.
Prow sidecar component will pick up these files collected in $ARTIFACTS path and show them in Artifacts tab.
see for examples -
https://cs.k8s.io/?q=ARTIFACTS&i=nope&literal=nope&files=&excludeFiles=&repos=kubernetes/test-infra
and docs - https://docs.prow.k8s.io/docs/components/pod-utilities/
There was a problem hiding this comment.
Changed. PTAL.
There was a problem hiding this comment.
@vitorfloriano, apologies for suggesting it in parts.
Since we're going to move/run all our test suites for NRC repo in Prow now, and all of those tests will have artifacts of interest, so, how about we make the change in the Makefile itself - https://github.com/kubernetes-sigs/node-readiness-controller/blob/2239ccef203f6be9af3209ae7b8d146b9fccfb55/Makefile
at the top of the makefile, define an env variable ARTIFACTS defaulting to path /logs/artifacts
And then in the test make target - we can capture the cover.out at path something like $ARTIFACTS/cover.out?
Same we can do for other tests as well as we move.
That way, we don't have to do this copy/pasting of each artifact file to ARTIFACTS here in the job definition.
WDYT?
There was a problem hiding this comment.
@Priyankasaggu11929 no problem. I thought about that at first, but then I considered that the make targets could be run locally and then I thought it would be better to move the artifacts here in the prowjob only.
But agreed, outputting to /logs/artifacts/cover.out seems more elegant. I'll open a PR in a minute.
vitorfloriano
force-pushed
the
prow-node-readiness-controller-presub-test
branch
from
ad959a5 to
f44eeab
Compare
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
vitorfloriano
force-pushed
the
prow-node-readiness-controller-presub-test
branch
from
f44eeab to
5d03968
Compare
vitorfloriano
force-pushed
the
prow-node-readiness-controller-presub-test
branch
from
5d03968 to
dad8884
Compare
vitorfloriano
force-pushed
the
prow-node-readiness-controller-presub-test
branch
2 times, most recently
from
1f4d01a to
8b16464
Compare
|
The changes appear good to me, leave to @Priyankasaggu11929 for final lgtm /assign @Priyankasaggu11929 |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ajaysundark, vitorfloriano The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
@vitorfloriano: Updated the
DetailsIn 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-sigs/prow repository. |
4 participants
This PR adds a presubmit job for running tests to kubernetes-sigs/node-readiness-controller.
Follow-up to kubernetes-sigs/node-readiness-controller#210 (comment)