[NoQA] Updating the isExpensifyEmployee action #8010

mountiny · 2022-03-04T15:57:55Z

Details

We are trying to update the new contributor comment to only show when the contributor is the author of the PR. Currently, we show the comment for Expensify employees' first PR.

Although the job code is basically the same as here:

App/.github/workflows/cherryPick.yml

Lines 15 to 33 in 1a351d3

    
           validateActor: 
        
             runs-on: ubuntu-latest 
        
             outputs: 
        
               IS_DEPLOYER: ${{ fromJSON(steps.isUserDeployer.outputs.isTeamMember) || github.actor == 'OSBotify' }} 
        
             steps: 
        
               - id: isUserDeployer 
        
                 uses: tspascoal/get-user-teams-membership@baf2e6adf4c3b897bd65a7e3184305c165aec872 
        
                 with: 
        
                   GITHUB_TOKEN: ${{ secrets.OS_BOTIFY_TOKEN }} 
        
                   username: ${{ github.actor }} 
        
                   team: mobile-deployers 
        
           createNewVersion: 
        
             needs: validateActor 
        
             runs-on: ubuntu-latest 
        
             if: ${{ fromJSON(needs.validateActor.outputs.IS_DEPLOYER) && github.event.inputs.NEW_VERSION == '' }} 
        
             outputs: 
        
               NEW_VERSION: ${{ steps.getNewVersion.outputs.NEW_VERSION }} 
        
             steps:

It seems it does not work as expected. The steps of the newContributorWelcomeMessage job are being run even for internal contributors, which should not happen.

I am just trying out in this PR, whether the same name of the output variable as the job is not causing this issue, currently, I do not know why it does not work.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/194440

Tests

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Verify that no errors appear in the JS console

I will take care of the QA. Same as here #7672

mountiny · 2022-03-04T16:12:28Z

Also, I have asked in Slack about another possible problem, which could be that the OS_BOTIFY does not have read access to the expensify-expensify team members and that is why it cant confirm.

mountiny · 2022-03-04T17:35:35Z

Updated to check for engineering team, per Slack thread here.

mountiny · 2022-03-04T18:04:32Z

Kindly asking you @AndrewGable for review as we discussed it in Slack, but no urgency here.

.github/workflows/lint.yml

chiragsalian

LGTM, the only thing that you've to change is what andrew requested.

mountiny · 2022-03-07T20:23:06Z

Yes, sorry for the delay, hindered with school today! Gonna have a look soon.

Co-authored-by: Andrew Gable <andrew@expensify.com>

mountiny · 2022-03-07T21:46:52Z

@chiragsalian @AndrewGable I have accepted the suggestion for expensify team, however, I am not completely sure it would work. Looking for team with such a name returns 404 https://github.com/orgs/Expensify/teams/expensify

AndrewGable · 2022-03-07T22:33:00Z

Yeah it didn't work 🤷

AndrewGable · 2022-03-07T22:42:44Z

Can we add more logs somehow? Or look in the action's repo for some help in investigation?

mountiny · 2022-03-08T01:55:02Z

It worked for the engineering team 🤔

mountiny · 2022-03-08T12:29:51Z

Ok, found the problem it is looking for Expensify/expensify team...

mountiny · 2022-03-08T13:05:13Z

Confirmed the Expensify/expensify team forws well so I will update the code and clean up!

mountiny · 2022-03-08T13:32:27Z

@chiragsalian @AndrewGable Alright, this should finally work. Thank you for your review!

Also thanks to @AndrewGable for proposing to use the lint for debugging, I did not know about this! Great to know :)

AndrewGable · 2022-03-08T16:34:40Z

.github/workflows/preDeploy.yml


  newContributorWelcomeMessage:
    runs-on: ubuntu-latest
    needs: isExpensifyEmployee
-    if: ${{ github.actor != 'OSBotify' && !fromJSON(needs.isExpensifyEmployee.outputs.isExpensifyEmployee) }}
+    if: ${{ github.actor != 'OSBotify' && !fromJSON(needs.isExpensifyEmployee.outputs.IS_EXPENSIFY_EMPLOYEE) }}


Should this be an OR instead of an AND?

OR would let OSBotify through no? OSBotify is not a member of the team so if it will be OR the other condition will always be true.

OSBotify · 2022-03-08T19:01:08Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

mountiny · 2022-03-09T18:04:34Z

Thanks for your help and patience on this one 😅

OSBotify · 2022-03-11T00:59:47Z

🚀 Deployed to staging by @chiragsalian in version: 1.1.42-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

roryabraham · 2022-03-11T01:02:09Z

Was anything done to verify if this worked? The team is called expensify-expensify and it's in the Expensify org:

Also, I had a contributor's first PR merged here yesterday and there was no welcome comment. See the job was skipped: https://github.com/Expensify/App/runs/5490903931?check_suite_focus=true

mountiny · 2022-03-11T01:11:58Z

@roryabraham yes, we run this in lint workflow to see what the output is and you can see it here where it works. I have printed out the contents of the teams which showed the team is called Expensify/expensify team (I dont know why) and then it worked well - returned true for me.

But I see what is worng there:

It seems the {{ github. actor }} is the one who merges it not the author of the PR as it treats the merge event.

mountiny · 2022-03-11T01:15:59Z

So I will need to use this step: steps.getMergedPullRequest.outputs.author to get the author.

roryabraham · 2022-03-11T01:16:02Z

Oh, nice catch. Send a PR and I will review 🙂

OSBotify · 2022-03-15T20:52:30Z

🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Rename the output

15addfe

mountiny requested a review from a team as a code owner March 4, 2022 15:57

mountiny self-assigned this Mar 4, 2022

MelvinBot requested review from chiragsalian and removed request for a team March 4, 2022 15:58

Check for engineering team

78c8b3d

mountiny added 2 commits March 4, 2022 17:49

Forgot to save the previous changes

8e80a48

Add debug to lint

31f4383

mountiny requested a review from AndrewGable March 4, 2022 18:04

AndrewGable reviewed Mar 4, 2022

View reviewed changes

.github/workflows/lint.yml Outdated Show resolved Hide resolved

chiragsalian previously approved these changes Mar 7, 2022

View reviewed changes

mountiny dismissed chiragsalian’s stale review via ad9ee82 March 7, 2022 21:44

Update .github/workflows/lint.yml

ad9ee82

Co-authored-by: Andrew Gable <andrew@expensify.com>

mountiny requested review from AndrewGable and chiragsalian March 7, 2022 21:46

mountiny added 2 commits March 8, 2022 12:16

Check what teams are returned for me

7adcb7b

Fetch upstream

6ec5d69

Debug the Expensify/expensify team

b3ceacf

Code clean up

0e0ebe5

AndrewGable reviewed Mar 8, 2022

View reviewed changes

AndrewGable approved these changes Mar 8, 2022

View reviewed changes

chiragsalian approved these changes Mar 8, 2022

View reviewed changes

chiragsalian merged commit 47b4541 into main Mar 8, 2022

chiragsalian deleted the vit-updateIsExpensifyEmployee branch March 8, 2022 18:51

OSBotify mentioned this pull request Mar 10, 2022

Deploy Checklist: New Expensify 2022-03-10 #8074

Closed

mountiny mentioned this pull request Mar 11, 2022

[NoQA] Fix the new contributor PR comment #8082

Merged

50 tasks

SumitDiyora mentioned this pull request Apr 20, 2022

Deploy Checklist: New Expensify 2022-04-05 #8492

Closed

22 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NoQA] Updating the isExpensifyEmployee action #8010

[NoQA] Updating the isExpensifyEmployee action #8010

mountiny commented Mar 4, 2022 •

edited

Loading

mountiny commented Mar 4, 2022

mountiny commented Mar 4, 2022

mountiny commented Mar 4, 2022

chiragsalian left a comment

mountiny commented Mar 7, 2022

mountiny commented Mar 7, 2022

AndrewGable commented Mar 7, 2022

AndrewGable commented Mar 7, 2022

mountiny commented Mar 8, 2022

mountiny commented Mar 8, 2022

mountiny commented Mar 8, 2022

mountiny commented Mar 8, 2022

AndrewGable Mar 8, 2022

mountiny Mar 8, 2022

OSBotify commented Mar 8, 2022

mountiny commented Mar 9, 2022

OSBotify commented Mar 11, 2022

roryabraham commented Mar 11, 2022 •

edited

Loading

mountiny commented Mar 11, 2022

mountiny commented Mar 11, 2022

roryabraham commented Mar 11, 2022 •

edited

Loading

OSBotify commented Mar 15, 2022

	validateActor:
	runs-on: ubuntu-latest
	outputs:
	IS_DEPLOYER: ${{ fromJSON(steps.isUserDeployer.outputs.isTeamMember) \|\| github.actor == 'OSBotify' }}
	steps:
	- id: isUserDeployer
	uses: tspascoal/get-user-teams-membership@baf2e6adf4c3b897bd65a7e3184305c165aec872
	with:
	GITHUB_TOKEN: ${{ secrets.OS_BOTIFY_TOKEN }}
	username: ${{ github.actor }}
	team: mobile-deployers

	createNewVersion:
	needs: validateActor
	runs-on: ubuntu-latest
	if: ${{ fromJSON(needs.validateActor.outputs.IS_DEPLOYER) && github.event.inputs.NEW_VERSION == '' }}
	outputs:
	NEW_VERSION: ${{ steps.getNewVersion.outputs.NEW_VERSION }}
	steps:

[NoQA] Updating the isExpensifyEmployee action #8010

[NoQA] Updating the isExpensifyEmployee action #8010

Conversation

mountiny commented Mar 4, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

mountiny commented Mar 4, 2022

mountiny commented Mar 4, 2022

mountiny commented Mar 4, 2022

chiragsalian left a comment

Choose a reason for hiding this comment

mountiny commented Mar 7, 2022

mountiny commented Mar 7, 2022

AndrewGable commented Mar 7, 2022

AndrewGable commented Mar 7, 2022

mountiny commented Mar 8, 2022

mountiny commented Mar 8, 2022

mountiny commented Mar 8, 2022

mountiny commented Mar 8, 2022

AndrewGable Mar 8, 2022

Choose a reason for hiding this comment

mountiny Mar 8, 2022

Choose a reason for hiding this comment

OSBotify commented Mar 8, 2022

mountiny commented Mar 9, 2022

OSBotify commented Mar 11, 2022

roryabraham commented Mar 11, 2022 • edited Loading

mountiny commented Mar 11, 2022

mountiny commented Mar 11, 2022

roryabraham commented Mar 11, 2022 • edited Loading

OSBotify commented Mar 15, 2022

mountiny commented Mar 4, 2022 •

edited

Loading

roryabraham commented Mar 11, 2022 •

edited

Loading

roryabraham commented Mar 11, 2022 •

edited

Loading