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 3 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
56 changes: 32 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,31 @@ 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 }}
author: ${{ steps.getMergedPullRequest.outputs.author }}
Comment on lines +52 to +57
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for handling the revert. I have tried to test this but at the moment, I have not quickly created required files. Here is a draft PR with a follow up.

For some reason the failed actions seem like it has not assigned labels to the outputs but probably any other. My guess is that the steps here must be wrapped in fromJSON to properly pass its value.

I will try to test this by creating a new workflow on merge and have a dummy PR merge it and see if the values are assigned correctly there just to make sure we do not have to revert anything there.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mountiny I also recommend that you try testing in https://github.com/Andrew-Test-Org/Public-Test-Repo, which is what I always do to workshop workflows I'm unsure about. I can give you admin access to merge your own PRs. Another option would be to try and get act working for a local test.

Copy link
Contributor Author

@mountiny mountiny Mar 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@roryabraham Ah, that is great! Didn't know about this testing org, I will try to utilize that. Act looks nice too, do you use it or do you prefer the test org for things you are unsure about? Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

I typically just use the test org


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 +80,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 +92,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 +225,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