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

[NoQA] Fix the new contributor PR comment #8082

Merged
merged 7 commits into from
Mar 16, 2022
Merged

[NoQA] Fix the new contributor PR comment #8082

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b66a976
Make sure to use the login of the PR author
mountiny Mar 11, 2022
b36c291
Refactor the getMergedPullRequest step into a separate job
mountiny Mar 11, 2022
67ebf69
Include author input in getMergedPullRequest
mountiny Mar 11, 2022
86ef870
Update .github/workflows/preDeploy.yml
mountiny Mar 15, 2022
47e4ac6
Merge branch 'main' into vit-fixIsExpensifyEmployee
mountiny Mar 16, 2022
c8ac032
Make the other jobs in preDeploy dependent directly on getMergedPullR…
mountiny Mar 16, 2022
602b76e
Merge branch 'vit-fixIsExpensifyEmployee' of github.com:Expensify/App…
mountiny Mar 16, 2022
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
55 changes: 31 additions & 24 deletions .github/workflows/preDeploy.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,30 @@ jobs:
GITHUB_TOKEN: ${{ github.token }}
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK }}

# Get details about the merged pull request
getMergedPullRequest:
runs-on: ubuntu-latest

outputs:
title: ${{ steps.getMergedPullRequest.outputs.title }}
body: ${{ steps.getMergedPullRequest.outputs.body }}
number: ${{ steps.getMergedPullRequest.outputs.number }}
labels: ${{ steps.getMergedPullRequest.outputs.labels }}
assignees: ${{ steps.getMergedPullRequest.outputs.assignees }}

steps:
- name: Get merged pull request
id: getMergedPullRequest
# TODO: Point back action actions-ecosystem after https://github.com/actions-ecosystem/action-get-merged-pull-request/pull/223 is merged
mountiny marked this conversation as resolved.
Show resolved Hide resolved
uses: roryabraham/action-get-merged-pull-request@7a7a194f6ff8f3eef58c822083695a97314ebec1
with:
github_token: ${{ secrets.GITHUB_TOKEN }}

chooseDeployActions:
runs-on: ubuntu-latest
needs: confirmPassingBuild
needs: [confirmPassingBuild, getMergedPullRequest]
outputs:
mergedPullRequest: ${{ steps.getMergedPullRequest.outputs.number }}
mergedPullRequest: ${{ fromJSON(needs.getMergedPullRequest.outputs.number) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally don't think it's a good practice for one job to parrot the output of another. Let's get rid of the the mergedPullRequest output from chooseDeployActions and instead make the version directly dependent upon the getMergedPullRequest job and reference it's output directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good eye! Makes sense to me to make the jobs as atomic as possible.

isStagingDeployLocked: ${{ steps.isStagingDeployLocked.outputs.IS_LOCKED }}
isAutomatedPullRequest: ${{ steps.isAutomatedPullRequest.outputs.IS_VERSION_BUMP_PR }}
shouldCherryPick: ${{ steps.shouldCherryPick.outputs.SHOULD_CHERRY_PICK }}
Expand All @@ -60,12 +79,6 @@ jobs:
fetch-depth: 0
token: ${{ secrets.OS_BOTIFY_TOKEN }}

- name: Get merged pull request
id: getMergedPullRequest
uses: actions-ecosystem/action-get-merged-pull-request@59afe90821bb0b555082ce8ff1e36b03f91553d9
with:
github_token: ${{ secrets.GITHUB_TOKEN }}

- name: Check if StagingDeployCash is locked
id: isStagingDeployLocked
uses: Expensify/App/.github/actions/isStagingDeployLocked@main
Expand All @@ -78,7 +91,7 @@ jobs:

- name: Check if merged pull request has `CP Staging` label
id: shouldCherryPick
run: echo "::set-output name=SHOULD_CHERRY_PICK::${{ contains(steps.getMergedPullRequest.outputs.labels, 'CP Staging') }}"
run: echo "::set-output name=SHOULD_CHERRY_PICK::${{ contains(fromJSON(needs.getMergedPullRequest.outputs.labels), 'CP Staging') }}"

skipDeploy:
runs-on: ubuntu-latest
Expand Down Expand Up @@ -211,49 +224,43 @@ jobs:
# Check if actor is member of Expensify organization by looking for Expensify/expensify team
isExpensifyEmployee:
runs-on: ubuntu-latest
needs: getMergedPullRequest

outputs:
IS_EXPENSIFY_EMPLOYEE: ${{ fromJSON(steps.checkActor.outputs.isTeamMember) }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to confirm here, when the output is from a step in the job, we do not have to use the fromJSON, right? It has been like that in other place.

The fromJSON is used if it is output across jobs?

IS_EXPENSIFY_EMPLOYEE: ${{ steps.checkActor.outputs.isTeamMember }}

steps:
- name: Check whether the actor is member of Expensify/expensify team
id: checkActor
uses: tspascoal/get-user-teams-membership@baf2e6adf4c3b897bd65a7e3184305c165aec872
with:
GITHUB_TOKEN: ${{ secrets.OS_BOTIFY_TOKEN }}
username: ${{ github.actor }}
username: ${{ fromJSON(needs.getMergedPullRequest.outputs.author) }}
team: Expensify/expensify

newContributorWelcomeMessage:
runs-on: ubuntu-latest
needs: isExpensifyEmployee
needs: [isExpensifyEmployee, getMergedPullRequest]
if: ${{ github.actor != 'OSBotify' && !fromJSON(needs.isExpensifyEmployee.outputs.IS_EXPENSIFY_EMPLOYEE) }}
steps:
# Version: 2.3.4
- uses: actions/checkout@5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f
with:
token: ${{ secrets.OS_BOTIFY_TOKEN }}

- name: Get merged pull request
id: getMergedPullRequest
# TODO: Point back action actions-ecosystem after https://github.com/actions-ecosystem/action-get-merged-pull-request/pull/223 is merged
uses: roryabraham/action-get-merged-pull-request@7a7a194f6ff8f3eef58c822083695a97314ebec1
with:
github_token: ${{ secrets.GITHUB_TOKEN }}

- name: Get PR count for ${{ steps.getMergedPullRequest.outputs.author }}
run: echo "PR_COUNT=$(gh pr list --author ${{ steps.getMergedPullRequest.outputs.author }} --state any | grep -c '')" >> "$GITHUB_ENV"
- name: Get PR count for ${{ fromJSON(needs.getMergedPullRequest.outputs.author) }}
run: echo "PR_COUNT=$(gh pr list --author ${{ fromJSON(needs.getMergedPullRequest.outputs.author) }} --state any | grep -c '')" >> "$GITHUB_ENV"
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

- name: Comment on ${{ steps.getMergedPullRequest.outputs.author }}\'s first pull request!
- name: Comment on ${{ fromJSON(needs.getMergedPullRequest.outputs.author) }}\'s first pull request!
if: ${{ fromJSON(env.PR_COUNT) == 1 }}
uses: actions-ecosystem/action-create-comment@cd098164398331c50e7dfdd0dfa1b564a1873fac
with:
github_token: ${{ secrets.OS_BOTIFY_TOKEN }}
number: ${{ steps.getMergedPullRequest.outputs.number }}
number: ${{ fromJSON(needs.getMergedPullRequest.outputs.number) }}
body: |
@${{ steps.getMergedPullRequest.outputs.author }}, Great job getting your first Expensify/App pull request over the finish line! :tada:
@${{ fromJSON(needs.getMergedPullRequest.outputs.author) }}, Great job getting your first Expensify/App pull request over the finish line! :tada:

I know there's a lot of information in our [contributing guidelines](https://github.com/Expensify/App/blob/main/CONTRIBUTING.md), so here are some points to take note of :memo::

Expand Down