testing strategy: add policy for presubmits #8196
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly 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 |
k8s-ci-robot
added
approved
|
/lgtm so Michelle has time to review, feel free to unhold |
k8s-ci-robot
added
do-not-merge/hold
|
We also need guidance about that fact that you shouldn't rely on path triggering to accurately reflect the inputs to something under test. IE you have a go tool and you have some extra tests for it that trigger on cmd/tool/.* I don't generally recommend this sort of non-blocking path based job and I'm struggling to think of a successful counter example Is this really a best practice? |
|
/hold I'm out for the holidays but will try to draft a comment on that test-infra PR soon. this approach is extremely error prone and has not gone well in the past for any attempt I've seen |
pohly
force-pushed
the
testing-strategy-optional-jobs
branch
from
a072efd to
e113ccd
Compare
k8s-ci-robot
removed
the
lgtm
|
New changes are detected. LGTM label has been removed. |
k8s-ci-robot
added
size/M
|
@BenTheElder I've tried to address both points in #8196 (comment) with https://github.com/kubernetes/community/compare/a072efd375511540ba0e8aba4845f4d7d133f0ad..e113ccd92f78a8c26a3166799c6aa7fc9352565d
I'm quite happy with this approach for DRA. It has served us (= the DRA developers) pretty well. I'm aware that this came at a cost for other developers (while transitioning between API versions, we had to break the jobs for master to make them work in the PR under review, and that showed up as test failures in other PRs), but I hope the feature justified that. If that hadn't been resolved quickly, the jobs should have been disabled because they didn't meet the criteria outlined in this PR. |
| has to be used explicitly for such non-blocking jobs. It is possible to configure such | ||
| jobs so that they [run automatically when certain paths are modified](https://github.com/kubernetes/test-infra/blob/ee70308f09c10f7cd933c26c98acc7ebf785d436/config/jobs/kubernetes/sig-node/sig-node-presubmit.yaml#L3201-L3202). | ||
|
|
||
| Use this with care! A non-blocking job that fails confuses other contributors |
There was a problem hiding this comment.
Consider making this a warning or other callout? (https://github.com/orgs/community/discussions/16925)
Warning
Critical content demanding immediate user attention due to potential risks.
There was a problem hiding this comment.
... and then I changed my mind after reading that discussion. It's beta (= could break) and limited to GitHub. I replaced it with a warning as in https://www.markdownguide.org/hacks/#admonitions (linked to by someone in that discussion).
|
|
||
| Note that such a job cannot detect all regressions. It's impossible to | ||
| define all the things that might cause a need to run tests (e.g. tool changes, | ||
| updates in packages that a feature depends on). Therefore it is required |
There was a problem hiding this comment.
Consider shouting this requirement out earlier in the section (maybe in a tl;dr at the top?)
There was a problem hiding this comment.
Done. Now the section first introduces the pros and cons, then ends with the guidelines.
pohly
force-pushed
the
testing-strategy-optional-jobs
branch
3 times, most recently
from
46d1a8a to
87af3a7
Compare
Blocking pre-submit jobs must be for stable, important features. Non-blocking pre-submit jobs should only be run automatically if they meet the criteria outlined in kubernetes/community#8196. To ensure that this is considered when defining pre-submit jobs, they need to be listed in `config/tests/jobs/presubmit-jobs.yaml`. UPDATE_FIXTURE_DATA=true re-generates that file to the expected state for inclusion in a PR.
Blocking pre-submit jobs must be for stable, important features. Non-blocking pre-submit jobs should only be run automatically if they meet the criteria outlined in kubernetes/community#8196. To ensure that this is considered when defining pre-submit jobs, they need to be listed in `config/tests/jobs/presubmit-jobs.yaml`. UPDATE_FIXTURE_DATA=true re-generates that file to the expected state for inclusion in a PR.
Blocking pre-submit jobs must be for stable, important features and must always run. Non-blocking pre-submit jobs should only be run automatically if they meet the criteria outlined in kubernetes/community#8196. To ensure that this is considered when defining pre-submit jobs, they need to be listed in `config/tests/jobs/policy/presubmit-jobs.yaml`. The OWNERS file in that new directory ensures that relevant reviewers need to approve. `make update-config-fixture` re-generates that file to the expected state for inclusion in a PR.
Blocking pre-submit jobs must be for stable, important features and must always run. Non-blocking pre-submit jobs should only be run automatically if they meet the criteria outlined in kubernetes/community#8196. To ensure that this is considered when defining pre-submit jobs, they need to be listed in `config/tests/jobs/policy/presubmit-jobs.yaml`. The OWNERS file in that new directory ensures that relevant reviewers need to approve. `make update-config-fixture` re-generates that file to the expected state for inclusion in a PR.
Blocking pre-submit jobs must be for stable, important features and must always run. Non-blocking pre-submit jobs should only be run automatically if they meet the criteria outlined in kubernetes/community#8196. To ensure that this is considered when defining pre-submit jobs, they need to be listed in `config/tests/jobs/policy/presubmit-jobs.yaml`. The OWNERS file in that new directory ensures that relevant reviewers need to approve. `make update-config-fixture` re-generates that file to the expected state for inclusion in a PR.
pohly
force-pushed
the
testing-strategy-optional-jobs
branch
from
87af3a7 to
e687f0a
Compare
pohly
changed the title
pohly
force-pushed
the
testing-strategy-optional-jobs
branch
from
e687f0a to
db1fa96
Compare
This was motivated in part by kubernetes/test-infra#33463 (comment) and is part of an effort to document best practices. The part about blocking presubmits and running them always is https://kubernetes.slack.com/archives/C2C40FMNF/p1734418617113169?thread_ts=1734417601.687079&cid=C2C40FMNF
pohly
force-pushed
the
testing-strategy-optional-jobs
branch
from
db1fa96 to
361b0e7
Compare
|
I did a major rewrite to cover presubmit jobs in general, not just those with The "blocking presubmits must always run" is enforced by kubernetes/test-infra#33958 |
Blocking pre-submit jobs must be for stable, important features and must always run. Non-blocking pre-submit jobs should only be run automatically if they meet the criteria outlined in kubernetes/community#8196. To ensure that this is considered when defining pre-submit jobs, they need to be listed in `config/tests/jobs/policy/presubmit-jobs.yaml`. The OWNERS file in that new directory ensures that relevant reviewers need to approve. `make update-config-fixture` re-generates that file to the expected state for inclusion in a PR.
Blocking pre-submit jobs must be for stable, important features and must always run. Non-blocking pre-submit jobs should only be run automatically if they meet the criteria outlined in kubernetes/community#8196. To ensure that this is considered when defining pre-submit jobs, they need to be listed in `config/tests/jobs/policy/presubmit-jobs.yaml`. The OWNERS file in that new directory ensures that relevant reviewers need to approve. `make update-config-fixture` re-generates that file to the expected state for inclusion in a PR.
This was triggered by
kubernetes/test-infra#33463 (comment) and is part of an effort to document best practices. The guidelines are cut-and-pasted from a discussion on Slack were we agreed that that they seem reasonable and should be documented publicly.
The part about blocking presubmits is based on https://kubernetes.slack.com/archives/C2C40FMNF/p1734418617113169?thread_ts=1734417601.687079&cid=C2C40FMNF
The guidelines are intentionally in a sub-section because then we can provide a deep link whenever this comes up in a test-infra PR review.
/assign @aojea @michelle192837