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

Consider increasing accuracy of pull request merging trigger #2

Open
JamesMGreene opened this issue Jan 19, 2022 · 2 comments
Open

Comments

@JamesMGreene
Copy link
Contributor

JamesMGreene commented Jan 19, 2022

We currently use pushes to the main branch as the trigger for checking that a pull request was merged:

https://github.com/githublearn/introduction-to-github/blob/60017ba97de7a27dcd93d192d238c00cc280dfb0/.github/workflows/4-merge-your-pull-request.yml#L7-L13

While it's true that merging a PR into the main branch will trigger in just such an event, it is also true that many other activities (like editing in the UI, other pushes, merging other PRs, etc.) could also result in that trigger. 😅

As such, we may want to consider increasing the accuracy of this event trigger if any folks start reporting such issues. 🤷🏻‍♂️

e.g. suggested:

on:
  # Note that supporting `workflow_dispatch` will require some extra logic checks if
  # if we switch to using `pull_request` instead of `push`!
  workflow_dispatch:
  pull_request:
    branches:
      - main
    types:
      - closed

Plus updating the job-level if from:

https://github.com/githublearn/introduction-to-github/blob/60017ba97de7a27dcd93d192d238c00cc280dfb0/.github/workflows/4-merge-your-pull-request.yml#L24-L29

to:

    if: ${{ github.repository_owner != 'githublearn' && github.event.pull_request.merged == true }}

We could also get specific about the name of the PR head branch, if that makes sense:

    if: >-
      ${{
        github.repository_owner != 'githublearn' &&
        github.event.pull_request.merged == true &&
        github.head_ref == 'pr_branch_name'
      }}

⚠️ To still support workflow_dispatch, we would need to tweak the if condition further as well, e.g. something like:

    if: >-
      ${{
        github.repository_owner != 'githublearn' &&
        (
          github.event_name == 'workflow_dispatch' ||
          (
            github.event.pull_request.merged == true &&
            github.head_ref == 'pr_branch_name'
          )
        )
      }}
@Vishaldsouza
Copy link

The code you have provided is an if statement that checks if the following conditions are met:

The repository owner is not githublearn.
The event name is either workflow_dispatch or the pull request has been merged and the head ref is pr_branch_name.
If all of these conditions are met, the if statement will execute the code that follows it.

This code could be used to run a workflow only if the repository owner is not githublearn and the event is either a workflow dispatch or a pull request that has been merged and the head ref is pr_branch_name.

For example, you could use this code to run a workflow that deploys a new version of your application to production only if the repository owner is not githublearn and the pull request has been merged and the head ref is pr_branch_name.

Here is a breakdown of the code:

if: >-
${{
github.repository_owner != 'githublearn' &&
(
github.event_name == 'workflow_dispatch' ||
(
github.event.pull_request.merged == true &&
github.head_ref == 'pr_branch_name'
)
)
}}
The first line of the code is the if keyword. This keyword tells GitHub that the following code should only be executed if the conditions inside the curly braces are met.

The next line of code is a nested conditional statement. This statement checks if the following conditions are met:

The repository owner is not githublearn.
The event name is either workflow_dispatch or the pull request has been merged and the head ref is pr_branch_name.
The && operator is used to combine the two conditions. This means that both conditions must be met for the nested conditional statement to be true.

If the nested conditional statement is true, the if statement will execute the code that follows it.

@Vzq666

This comment has been minimized.

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

No branches or pull requests

7 participants
@JamesMGreene @Vishaldsouza @Vzq666 and others