Suggestion: Skip Prow CI tests if PR contains changes only in `site/` directory about prow HOT 11 OPEN

From an implementation perspective:
https://docs.prow.k8s.io/docs/jobs/#triggering-jobs

skip_if_only_changed can be used, however different repos need different paths.

I recommend being careful with this because you might have surprise interactions between paths you didn't think were relevant, and limiting it to only something like skip_if_only_changed: ^site/ for unit tests etc seems reasonable.

A maintainer of this repo will need to approve the config changes.

from prow.

Hello there!!

I agree with your suggestion!! However, some parts of the proposal seem to have already been implemented. FYI:

https://github.com/kubernetes/test-infra/blob/fb89093a280e0af2fed549399fb17cfb66a7bf9a/config/jobs/kubernetes-sigs/prow/prow-presubmits.yaml#L2-L5

https://github.com/kubernetes/test-infra/blob/fb89093a280e0af2fed549399fb17cfb66a7bf9a/config/jobs/kubernetes-sigs/prow/prow-presubmits.yaml#L94-L96

I am not sure if Netlify's deploy preview supports this feature, but are there other Kubernetes or CNCF repos that could serve as a reference for implementing this suggestion?? thanks

from prow.

Your suggestion seems very reasonable @BenTheElder.

Do you think it would be better to remove the skip_if_only_changed: '^site/' setting from the pull-prow-integration and pull-prow-image-build-test?

from prow.

Sorry I missed this.

Do you think it would be better to remove the skip_if_only_changed: '^site/' setting from the pull-prow-integration and pull-prow-image-build-test?

It depends on how confident we are that whatever runs in those will not ever have a dependency on site/.

If we have e.g. a spelling check or something like that, we'd still want to include site/

But probably e.g. unit tests or prow images do not ever need to run when site/ changes.

from prow.

I completely agree with your suggestion. It seems that spellcheck (or like this) are not included. Excluding the /site from some CI would be a good first step.

If there are no objections or barriers within approximately 48 hours, I would like to work on this change. Alternatively, anyone is free to work it on.

from prow.

In k-sigs/prow, Unit Test CI is marked as "Required." According to the documentation, it seems that if tests are run conditionally, they cannot be marked as required. Since it doesn't seem possible to remove the "Required" status for Unit Test, it appears challenging to run these tests conditionally.

I think it's gonna be fine (eventually) - iirc the branchprotector sets the job as required because it is unconditionally triggered (until your PR lands). Once it is conditionally triggered, next branchprotector run should remove the BP config.

from prow.

Sounds good to me

from prow.

/sig contributor-experience

from prow.

/assign

opened kubernetes/test-infra#33134 - please have a look :)

In k-sigs/prow, Unit Test CI is marked as "Required." According to the documentation, it seems that if tests are run conditionally, they cannot be marked as required. Since it doesn't seem possible to remove the "Required" status for Unit Test, it appears challenging to run these tests conditionally.

FYI: https://docs.prow.k8s.io/docs/jobs/#requiring-job-statuses

Protecting Status Contexts

The branch protection rules will only enforce the presence of jobs that run unconditionally and have required status contexts. As conditionally-run jobs may or may not post a status context to GitHub, they cannot be required through this mechanism.

from prow.

I see, so we should focus on whether or not Prow should trigger unit tests conditionally.

I think Prow doesn’t need to trigger unit tests when only site/ is changed. WDYT?

from prow.

Thanks, updated kubernetes/test-infra#33134, please take a look!

from prow.