Add condtional checks on jobs to skip on document changes. #5438
Conversation
|
This PR has multiple commits, and the default merge method is: merge. 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. |
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
size/S
Add checkout
sarab97
force-pushed
the
act-skip-cond
branch
from
b4e66bc to
fefa21f
Compare
sarab97
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
| with: | ||
| filters: | | ||
| doc: | ||
| - 'site/**' |
There was a problem hiding this comment.
Thanks @sarab97 for the PR!
I suppose, we have to ignore:
- somePath/*_test.go files
- somePath/*.md files
- and somePath/.gitignore files
I'm not sure what "Diff the API" means referred on the issue page, will ask further clarification on this and get back to you
There was a problem hiding this comment.
I have confirmed that we need to add the condition for this job as well
// .github/workflows/apidiff.yml
jobs:
go-apidiff:
name: Verify API differences
runs-on: ubuntu-latest
...There was a problem hiding this comment.
I have confirmed that we need to add the condition for this job as well
// .github/workflows/apidiff.yml jobs: go-apidiff: name: Verify API differences runs-on: ubuntu-latest ...
Yes, I was looking through that and intentionally skipped that. As its single job in a different workflow so adding this condition would mean adding the same conditional job there too. And that will always run to check condition and then make decision to skip that single api diff job or not. I took judgement call to skip doing this there as incase of code changes we will be spending additional time and resources to the same workflow and incase of document changes a single job will run anyways crawling though the code. Im open to your suggestions and we surely can integrate it there as well.
There was a problem hiding this comment.
Thanks @sarab97 for the PR!
I suppose, we have to ignore:
* somePath/*_test.go files * somePath/*.md files * and somePath/.gitignore filesI'm not sure what "Diff the API" means referred on the issue page, will ask further clarification on this and get back to you
Im not sure adding *_test.go files falls under doc changes scope. However *md file does make sense. I was thinking maybe we first stablize this then we can easily add any more conditions as per need.
"diif the api" refers to that apidiff workflow
There was a problem hiding this comment.
Yes, I was looking through that and intentionally skipped that. As its single job in a different workflow so adding this condition would mean adding the same conditional job there too. And that will always run to check condition and then make decision to skip that single api diff job or not. I took judgement call to skip doing this there as incase of code changes we will be spending additional time and resources to the same workflow and incase of document changes a single job will run anyways crawling though the code. Im open to your suggestions and we surely can integrate it there as well.
Let's add the condition there in case apidiff workflow expands in the future. Or maybe we can make condition check job generic and can be referenced from these workflows, maybe we can use these strategies:
- https://github.com/marketplace/actions/repository-dispatch
- https://stackoverflow.com/questions/73702333/github-actions-reuse-outputs-from-other-reusable-workflows
Im not sure adding *_test.go files falls under doc changes scope. However *md file does make sense. I was thinking maybe we first stablize this then we can easily add any more conditions as per need.
"diif the api" refers to that apidiff workflow
Ah you are right, I misunderstood the requirements, you don't need to ignore *_test after all. Speaking of detail of the check, does a code comment change counts as code change or docs change?
There was a problem hiding this comment.
Let's add the condition there in case apidiff workflow expands in the future. Or maybe we can make condition check job generic and can be referenced from these workflows, maybe we can use these strategies:
The apidiff is a single action workflow having a job. A workflow has single purpose. So checking for api diff that wont be expanding.
I have looked though link you provided. Those do seem interesting approach but have their own drawbacks like rerunning it again for each call which will be inefficient in our case.
I would prefer keeping this simple rather than overcomplicating.
Ah you are right, I misunderstood the requirements, you don't need to ignore *_test after all. Speaking of detail of the check, does a code comment change counts as code change or docs change?
We dont need to go in so deep wondering about comment change. We wont be having any way to check that. And even if possible it would be too much of effort for too little of a reward.
There was a problem hiding this comment.
Okay, let's just make sure the condition cover *.md and .gitignore for now
There was a problem hiding this comment.
Yeah @antoooks I left it out to be done in future. But sure I will add both conditions.
|
/lgtm |
|
@antoooks: changing LGTM is restricted to collaborators In 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/test-infra repository. |
|
/assign |
| lint: | ||
| name: Lint | ||
| needs: conditional-changes | ||
| if: needs.conditional-changes.outputs.doc == 'false' |
There was a problem hiding this comment.
@sarab97
Thanks for your contribution!
So, I think it looks like this condition checks only whether site/ was changed or not.
I worry that maybe this condition won't work with any PR changing code and documents.
Please tell me if I misunderstood your work!
There was a problem hiding this comment.
As per my understanding of README from https://github.com/dorny/paths-filter. The condition will check for change and as per filter condition ensure the change is only in site directory. So if both document and code is changed it should evaluate to false.
There was a problem hiding this comment.
https://github.com/dorny/paths-filter?tab=readme-ov-file#outputs
'false' - if none of changed files matches any of filter rules
Oh. Thanks I understand.
I misunderstand false as meaning the reverse of true.
k8s-ci-robot
added
the
approved
|
Thanks @sarab97. This PR looks good. /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: koba1t, sarab97 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 |
Fixes #5305
Tested on PR
This approach is used to ensure that workflow is run but jobs are skipped as this is how required github actions work. Link to github documentation