Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add condtional checks on jobs to skip on document changes. #5438

Merged
merged 1 commit into from
Feb 13, 2024
Merged

Add condtional checks on jobs to skip on document changes. #5438

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions .github/workflows/go.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,25 @@ permissions:
contents: read

jobs:
conditional-changes:
runs-on: ubuntu-latest
permissions:
pull-requests: read
outputs:
doc: ${{ steps.filter.outputs.doc }}
steps:
- uses: actions/checkout@v3
- uses: dorny/paths-filter@v2
id: filter
with:
filters: |
doc:
- 'site/**'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's just make sure the condition cover *.md and .gitignore for now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah @antoooks I left it out to be done in future. But sure I will add both conditions.


lint:
name: Lint
needs: conditional-changes
if: needs.conditional-changes.outputs.doc == 'false'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

runs-on: [ubuntu-latest]
steps:
- name: Check out code into the Go module directory
Expand All @@ -30,6 +47,8 @@ jobs:

test-linux:
name: Test Linux
needs: conditional-changes
if: needs.conditional-changes.outputs.doc == 'false'
runs-on: [ubuntu-latest]
steps:
- name: Check out code into the Go module directory
Expand All @@ -46,6 +65,8 @@ jobs:

test-macos:
name: Test MacOS
needs: conditional-changes
if: needs.conditional-changes.outputs.doc == 'false'
runs-on: [macos-latest]
steps:
- name: Check out code into the Go module directory
Expand All @@ -62,6 +83,8 @@ jobs:

test-windows:
name: Test Windows
needs: conditional-changes
if: needs.conditional-changes.outputs.doc == 'false'
runs-on: [windows-latest]
steps:
- name: Check out code into the Go module directory
Expand Down
Loading