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

fix: don’t build anew if a PR was simply edited, edited again #336

Merged
merged 6 commits into from
Oct 12, 2022

Conversation

jenstroeger
Copy link
Owner

See also issue #330: there’s no need to run the build.yaml workflow if a PR was merely edited (i.e. no code was changed).

@jenstroeger jenstroeger requested a review from behnazh October 4, 2022 14:01
@jenstroeger jenstroeger force-pushed the dont-build-if-pr-edited branch from e1fdcb7 to 8f48a95 Compare October 4, 2022 14:04
@jenstroeger
Copy link
Owner Author

Hm. I wonder if this is a good idea 🤔

Suppose we create a PR and the pull-request.yaml workflow fails because build.yaml fails; next, we make a comment on that same PR and now the workflow succeeds because it wouldn’t build again…

@jenstroeger jenstroeger changed the title fix: don’t build anew if a PR was simply edited fix: test me! Oct 4, 2022
@jenstroeger
Copy link
Owner Author

jenstroeger commented Oct 4, 2022

Ok, run 3182767662 (6m 36s) was the initial run when we opened this PR which built the artifacts. Then we changed the PR title, thus triggering run 3182834503 (20s). Next, let’s change the title back.

Adding comments to a PR doesn’t seem to trigger the workflow, though.

@jenstroeger jenstroeger changed the title fix: test me! fix: don’t build anew if a PR was simply edited Oct 4, 2022
@behnazh
Copy link
Collaborator

behnazh commented Oct 5, 2022

Hm. I wonder if this is a good idea thinking

Suppose we create a PR and the pull-request.yaml workflow fails because build.yaml fails; next, we make a comment on that same PR and now the workflow succeeds because it wouldn’t build again…

How about creating two separate workflows for checking PR title and PR code changes to avoid the issue?

@jenstroeger jenstroeger force-pushed the dont-build-if-pr-edited branch from 8f48a95 to 78edf05 Compare October 8, 2022 01:44
@jenstroeger
Copy link
Owner Author

How about creating two separate workflows for checking PR title and PR code changes to avoid the issue?

Commit 78edf05.

@jenstroeger jenstroeger changed the title fix: don’t build anew if a PR was simply edited fix: don’t build anew if a PR was simply edited, edited again Oct 9, 2022
# This workflow checks and tests the package code, and it builds all package
# artifacts whenever there were changes to a pull request.

name: 'Pull Request: change set'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Pull Request in the name is not necessary because the trigger should appear in the workflow run.

How about "Check change set"?

Copy link
Owner Author

@jenstroeger jenstroeger Oct 11, 2022

Choose a reason for hiding this comment

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

On the website the action run shows the name: of the Action:

actions

with

name: Scorecards supply-chain security

How about "Check change set"?

Sure.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Commit bd8d88b.

@@ -3,7 +3,7 @@
# tool to check the title of the PR and all commit messages of the branch
# which triggers this Action.

name: Pull Request
name: 'Pull Request: conventional commits'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment regarding the name.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Commit bd8d88b.

@jenstroeger jenstroeger requested a review from behnazh October 11, 2022 21:52
pull_request:
branches:
- main
- staging
Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed shall we change the target branches to * to make sure the checks run on all pull requests?

Copy link
Owner Author

Choose a reason for hiding this comment

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

This PR or a separate one?

@jenstroeger jenstroeger merged commit cffd369 into staging Oct 12, 2022
@jenstroeger jenstroeger deleted the dont-build-if-pr-edited branch October 30, 2022 05:54
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