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.

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

ci: use dorny/paths-filter to filter paths when running go-tests #3675

Merged
merged 3 commits into from
Sep 7, 2023

Conversation

ghaiszaher
Copy link
Contributor

@ghaiszaher ghaiszaher commented Aug 16, 2023

This is just a proposal / discussion starter

what

Instead of having 2 separate workflow files, one that runs tests based on changed paths and the other runs on other changed files, have only one workflow that makes sure to either run tests or no tests.

why

  • The issue with current behavior is when pull requests change go and non-go files, we'll have 2 workflows triggered and both fighting for the concurrency lock, whoever wins executes.
  • Using on.*.paths, there is no way to ensure test-required.yml is only run if test.yml is skipped (i.e. always give priority to test.yml)

Examples of when test.yml failed to run while there are changed go files:

PR test.yml skipped test-required.yml taking over Note
#3428 runs/5762924585 runs/5762924587/job/15623661330 Was merged although tests are failing
#2782 runs/5872347855 runs/5872347868/job/15923694053
#3451 runs/5490971081 runs/5490971079/job/14871640586

tests

I tested in ghaiszaher#51

drawbacks

  • 2 required checks appear in pull requests, but only one of them passing is enough:
    image
    (I still see it better than risking skipping tests when they are actually failing)

alternatives

  • Always run tests

references

@ghaiszaher ghaiszaher marked this pull request as ready for review August 18, 2023 14:05
@ghaiszaher ghaiszaher requested a review from a team as a code owner August 18, 2023 14:05
@jamengual
Copy link
Contributor

@GenPage I defer this one to you since you have done considerable amount of work on the workflows

@GenPage GenPage added this to the v0.26.0 milestone Aug 31, 2023
@GenPage GenPage added the waiting-on-review Waiting for a review from a maintainer label Aug 31, 2023
@nitrocode
Copy link
Member

As the original writer of #3123 of the negative tests, as (previously) recommended by github docs, FWIW, I'm completely OK with removing the negative tests which are finicky and tricky to debug. This proposed method of using a separate action to do the path filtering will reduce maintenance and make it easier to troubleshoot the triggered action.

Copy link
Member

@GenPage GenPage left a comment

Choose a reason for hiding this comment

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

I think this will be more consistent in the long-run, thank you for proposing this.

@GenPage GenPage merged commit fac46b7 into runatlantis:main Sep 7, 2023
14 checks passed
@ghaiszaher ghaiszaher deleted the ci/path-filters branch September 7, 2023 16:43
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github-actions waiting-on-review Waiting for a review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix: Investigate why renovate PRs are not running the full CI
4 participants