tide, Fix require_manually_triggered_jobs tide handling #31687

oshoval · 2024-01-21T15:52:29Z

When require_manually_triggered_jobs is used, tide doesn't wait for the jobs
that require manual trigger, nor adds them to batches.
This PR fix it, by checking the bp settings, and if the require_manually_triggered_jobs feature is enabled
for the branch, then force the jobs that have the following settings:

always_run=false
optional=false
skip_report=false
run_if_changed and skip_if_only_changed are empty

or has run_before_merge=true (as was already before this PR).

Fixes: #31686
Fixes: #31050

linux-foundation-easycla · 2024-01-21T15:52:35Z

The committers listed above are authorized under a signed CLA.

✅ login: oshoval / name: oscollabus (7cf1e9a)

k8s-ci-robot · 2024-01-21T15:52:38Z

Hi @oshoval. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

oshoval · 2024-01-21T15:53:44Z

Hi @jmguzik @petr-muller
Can you please suggest if the direction is fine ?
I will add unit tests of course once we see it is on the right track

Thanks

petr-muller · 2024-01-24T16:42:29Z

/ok-to-test

I think the idea is sound, happy to review when out of WIP and polished

oshoval · 2024-01-30T12:48:03Z

Thanks
Added unit tests
PR ready for review please

jmguzik

Many thanks for handling this edge-case. I added some comments.

prow/tide/tide.go

prow/tide/tide_test.go

prow/tide/tide.go

oshoval · 2024-01-31T10:38:29Z

Thanks
Addressed comments

jmguzik

From my POV
/lgtm
/assign @petr-muller
Let's wait on @petr-muller approval.

oshoval · 2024-01-31T10:54:47Z

Thanks

pull-test-infra-unit-test-race-detector-nonblocking isn't blocking iiuc, and also not due to this PR
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/test-infra/31687/pull-test-infra-unit-test-race-detector-nonblocking/1752642590001336320

For visibility, there is the function GetBranchProtection, but in case the Org is not found, it doesn't fall back to the global BP settings, this why i didn't use it.

Btw not part of this PR scope, but aint it redundant that GetTideContextPolicy and presubmitsByPull have lots in common but still different implementation even on the common parts, and also their result is populated into different elements ?
(I am totally not into every detail, so of course i might miss the point)

droslean · 2024-01-31T11:01:30Z

/retest

Tide didn't take into account jobs that require manual triggering, on batches and when populating the subpool presubmits. It resulted in missing protection for this kind of jobs. Fix it by acting according the policy, and including require manual triggering jobs in tide decisions. Signed-off-by: Or Shoval <oshoval@redhat.com>

oshoval · 2024-01-31T11:04:40Z

oops commit had the word WIP, removed it
btw aint it should be blocked if there is a word WIP on commit title ?

petr-muller

Thanks!

k8s-ci-robot · 2024-02-03T15:24:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmguzik, oshoval, petr-muller

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:

~~prow/OWNERS~~ [petr-muller]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

petr-muller · 2024-02-03T15:25:46Z

btw aint it should be blocked if there is a word WIP on commit title ?

I think the plugin only checks PR title, not commits

Now that kubernetes/test-infra#31687 is merged, we don't need the workaround anymore. Required manually triggered jobs would be detected as required by tide, and have the same effect as run_before_merge. Signed-off-by: Or Shoval <oshoval@redhat.com>

…in (#3221) * phased plugin: Enable from-branch-protection Signed-off-by: Or Shoval <oshoval@redhat.com> * phased plugin: Remove run_before_merge workaround Now that kubernetes/test-infra#31687 is merged, we don't need the workaround anymore. Required manually triggered jobs would be detected as required by tide, and have the same effect as run_before_merge. Signed-off-by: Or Shoval <oshoval@redhat.com> --------- Signed-off-by: Or Shoval <oshoval@redhat.com>

…in (kubevirt#3221) * phased plugin: Enable from-branch-protection Signed-off-by: Or Shoval <oshoval@redhat.com> * phased plugin: Remove run_before_merge workaround Now that kubernetes/test-infra#31687 is merged, we don't need the workaround anymore. Required manually triggered jobs would be detected as required by tide, and have the same effect as run_before_merge. Signed-off-by: Or Shoval <oshoval@redhat.com> --------- Signed-off-by: Or Shoval <oshoval@redhat.com>

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 21, 2024

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 21, 2024

k8s-ci-robot requested review from cjwagner and jmguzik January 21, 2024 15:52

k8s-ci-robot added area/prow Issues or PRs related to prow area/prow/tide Issues or PRs related to prow's tide component sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jan 21, 2024

oshoval mentioned this pull request Jan 21, 2024

phased: Enable run_before_merge for phase 2 jobs kubevirt/project-infra#3184

Merged

oshoval force-pushed the fix_rm branch from ac3b046 to dd2ae7f Compare January 22, 2024 08:56

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 22, 2024

This was referenced Jan 22, 2024

Tide doesn't run NeedsExplicitTrigger jobs on batch, when require_manually_triggered_jobs bp is enabled #31686

Closed

Update test-infra dependency to make use of new prowjob config fields kubevirt/project-infra#3202

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 24, 2024

oshoval force-pushed the fix_rm branch from dd2ae7f to 1f67fa7 Compare January 30, 2024 12:44

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 30, 2024

oshoval force-pushed the fix_rm branch from 1f67fa7 to f39525e Compare January 30, 2024 12:44

oshoval changed the title ~~WIP: Fix require_manually_triggered_jobs tide handling~~ tide, Fix require_manually_triggered_jobs tide handling Jan 30, 2024

oshoval marked this pull request as ready for review January 30, 2024 12:44

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 30, 2024

k8s-ci-robot requested a review from droslean January 30, 2024 12:45

k8s-ci-robot removed the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 30, 2024

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 30, 2024

jmguzik reviewed Jan 31, 2024

View reviewed changes

prow/tide/tide.go Show resolved Hide resolved

prow/tide/tide_test.go Outdated Show resolved Hide resolved

prow/tide/tide.go Outdated Show resolved Hide resolved

oshoval force-pushed the fix_rm branch from f39525e to 92ee7d1 Compare January 31, 2024 10:38

jmguzik approved these changes Jan 31, 2024

View reviewed changes

k8s-ci-robot assigned petr-muller and jmguzik Jan 31, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2024

oshoval force-pushed the fix_rm branch from 92ee7d1 to 7cf1e9a Compare January 31, 2024 11:04

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2024

petr-muller approved these changes Feb 3, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2024

k8s-ci-robot merged commit 43e4bce into kubernetes:master Feb 3, 2024
7 checks passed

This was referenced Feb 4, 2024

phased plugin: Enable from-branch-protection for kubevirt/kubevirt main kubevirt/project-infra#3221

Merged

Bump prow kubevirt/project-infra#3234

Closed

oshoval mentioned this pull request Feb 26, 2024

Sig-storage ownership kubevirt/kubevirt#11210

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tide, Fix require_manually_triggered_jobs tide handling #31687

tide, Fix require_manually_triggered_jobs tide handling #31687

oshoval commented Jan 21, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jan 21, 2024 •

edited

Loading

k8s-ci-robot commented Jan 21, 2024

oshoval commented Jan 21, 2024 •

edited

Loading

petr-muller commented Jan 24, 2024

oshoval commented Jan 30, 2024 •

edited

Loading

jmguzik left a comment

oshoval commented Jan 31, 2024

jmguzik left a comment

oshoval commented Jan 31, 2024 •

edited

Loading

droslean commented Jan 31, 2024

oshoval commented Jan 31, 2024

petr-muller left a comment

k8s-ci-robot commented Feb 3, 2024

petr-muller commented Feb 3, 2024

tide, Fix require_manually_triggered_jobs tide handling #31687

tide, Fix require_manually_triggered_jobs tide handling #31687

Conversation

oshoval commented Jan 21, 2024 • edited Loading

linux-foundation-easycla bot commented Jan 21, 2024 • edited Loading

k8s-ci-robot commented Jan 21, 2024

oshoval commented Jan 21, 2024 • edited Loading

petr-muller commented Jan 24, 2024

oshoval commented Jan 30, 2024 • edited Loading

jmguzik left a comment

Choose a reason for hiding this comment

oshoval commented Jan 31, 2024

jmguzik left a comment

Choose a reason for hiding this comment

oshoval commented Jan 31, 2024 • edited Loading

droslean commented Jan 31, 2024

oshoval commented Jan 31, 2024

petr-muller left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 3, 2024

petr-muller commented Feb 3, 2024

oshoval commented Jan 21, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jan 21, 2024 •

edited

Loading

oshoval commented Jan 21, 2024 •

edited

Loading

oshoval commented Jan 30, 2024 •

edited

Loading

oshoval commented Jan 31, 2024 •

edited

Loading