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

chore(workflow): specific target directory in Test process action #69

Merged
merged 4 commits into from
Dec 20, 2024

Conversation

HsiangNianian
Copy link
Collaborator

No description provided.

@HsiangNianian HsiangNianian enabled auto-merge (squash) December 19, 2024 01:36
Copy link
Contributor

github-actions bot commented Dec 19, 2024

@fu050409
Copy link
Member

@HsiangNianian Thanks for you contribution, however, we're using required workflow to ensure we won't merge some pull requests contains issues unexpectedly, which depend on GitHub branch rules. I think we should not do this.

@HsiangNianian HsiangNianian enabled auto-merge (squash) December 19, 2024 03:22
@HsiangNianian
Copy link
Collaborator Author

HsiangNianian commented Dec 19, 2024

@HsiangNianian Thanks for you contribution, however, we're using required workflow to ensure we won't merge some pull requests contains issues unexpectedly, which depend on GitHub branch rules. I think we should not do this.

TL;DR I understand what you mean, and I highly recommend this approach, please allow me to explain this PR

In actions, especially push and pull_request events, paths have been added to indicate that the Test workflow will only be triggered when the commit path contains the src or test folders (i.e., both have been modified)

This is done to reduce the unnecessary workflow overhead caused by file changes under other paths

At the same time, in the community, we should strive to promote this type of workflow, workflows are event-based, but you still need to clearly distinguish between the project directories corresponding to different workflows and the different paths and partitions in the project architecture, look at the following e.g.(workflow architecture for documents updates):

name: Build Docs

on:
    push:
        - "docs/"

# ...

You will benefit from this, but dont panic, you can add or remove paths at any time :D

@fu050409
Copy link
Member

fu050409 commented Dec 19, 2024

@HsiangNianian I have to say that I agree with your idea. I add this path check for my personal account too, however, I have to remove branch protect rule for ci checks since PRs without test checks cannot be merged. But there is no workaround to add a GitHub based rule to avoid our maintainers merging PRs that failed on test checks without branch protect rule.

@fu050409 fu050409 disabled auto-merge December 19, 2024 03:49
@HsiangNianian
Copy link
Collaborator Author

HsiangNianian commented Dec 19, 2024

@HsiangNianian I have to say that I agree with your idea. I add this path check for my personal account too, however, I have to remove branch protect rule for ci checks since PRs without test checks cannot be merged. But there is no workaround to add a GitHub based rule to avoid our maintainers merging PRs that failed on test checks without branch protect rule.

I understand your concerns, but this is a harmless thing. You can try it. I suspect two outcomes:

  1. The checks continue but are blocked or skipped. The contributor cannot set up automatic merger.

  2. there is no test check(or Pending), PR can be automatically merged after other workflows checked

I would prefer the second scenario, at the same time, you need to understand what workflow audits mean, and you can request a corresponding workflow for changes to files in other paths which are not relevant for development (although it doesn't mean too much)

@fu050409
Copy link
Member

@HsiangNianian Here's what actually happens:

  1. If we keep GitHub's branch protection policy in place, then if a change in a pr is not within the specified path, the corresponding ci check is not triggered. For example, we have a test check, which means that no member has permission to merge a pr that doesn't trigger this task, since passing the test check is mandatory. Even if it's not triggered, GitHub will assume that the pr didn't pass the expected check and block all merges.

  2. If we remove the corresponding mandatory branch protection to allow PRs that don't triger a test check to be merged, then our maintainer might accidentally merge a pr that triggers but fails the test, and GitHub won't block the merge because passing the test isn't mandatory, even if it fails.

In the last change I think it makes sense, this at least reduces the overhead of the main branch. Overall, thanks for your contribution.

@fu050409 fu050409 merged commit 48edf26 into main Dec 20, 2024
2 checks passed
@fu050409 fu050409 deleted the HsiangNianian-patch-1 branch December 20, 2024 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants