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

Jump to bottom

[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

merged 7 commits into from
Mar 16, 2022

Conversation

mountiny
Copy link
Contributor

@mountiny mountiny commented Mar 11, 2022 •
edited
Loading

Follow up to #8010

Details

Adding the additional step to get details about the PR and use the author of the PR and not the person who is merging the PR.

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

  • I verified the PR has a small number of commits behind main
  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I clearly indicated the environment tests should be run in (Staging vs Production)
  • I wrote testing steps that cover success & fail scenarios (if applicable)
  • I included screenshots or videos for tests on all platforms
  • I ran the tests & veryfy they passed on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors related to changes in this PR
  • I followed proper code patterns (see Reviewing the code)
    • I added comments when the code was not self explanatory
    • I put all copy / text shown in the product in all src/languages/* files (if applicable)
    • I followed proper naming convention for platform-specific files (if applicable)
    • I followed style guidelines (in Styling.md) for all style edits I made
    • I followed the JSDocs style guidelines (in STYLE.md)
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I corroborated the UI performance was not affected (the performance is the same than main branch)
  • If I created a new component I verified that a similar component doesn't exist in the codebase

PR Reviewer Checklist

  • I verified the PR has a small number of commits behind main
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the testing environment is mentioned in the test steps
  • I verified testing steps cover success & fail scenarios (if applicable)
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors related to changes in this PR
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified comments were added when the code was not self explanatory
    • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
    • I verified proper naming convention for platform-specific files was followed (if applicable)
    • I verified style guidelines were followed
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified the UI performance was not affected (the performance is the same than main branch)
  • If a new component is created I verified that a similar component doesn't exist in the codebase

QA Steps

  • Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@mountiny mountiny requested a review from roryabraham March 11, 2022 01:24
@mountiny mountiny requested a review from a team as a code owner March 11, 2022 01:24
@mountiny mountiny self-assigned this Mar 11, 2022
@MelvinBot MelvinBot requested review from cead22 and removed request for a team March 11, 2022 01:24
@mountiny
Copy link
Contributor Author

@roryabraham PR is here and we can make sure this works after you merge it and then the logs should mention mountiny here:
image

@mountiny
Copy link
Contributor Author

@roryabraham So not sure what are the best practices for the actions here, but now the getMergedPullRequest is reused in three jobs in this workflow: chooseDeployActions, isExpensifyEmployee and newContributorWelcomeMessage.

Should this action be separated to its own job which will then be included in needs by the other jobs to call it just once?

@roryabraham
Copy link
Contributor

Should this action be separated to its own job which will then be included in needs by the other jobs to call it just once?

Sounds like a good idea to DRY things up. You'll need to make sure that the job has all the correct outputs that are needed by other jobs that reference this step.

mountiny
mountiny commented Mar 11, 2022
View reviewed changes
.github/workflows/preDeploy.yml

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?

@mountiny mountiny changed the title [NoQA] Fix the new contributor PR comment [WIP] [NoQA] Fix the new contributor PR comment Mar 11, 2022
@mountiny mountiny changed the title [WIP] [NoQA] Fix the new contributor PR comment [NoQA] Fix the new contributor PR comment Mar 11, 2022
@mountiny
Copy link
Contributor Author

@roryabraham updated! and left one question, thank you very much for having a look!

cead22
cead22 requested changes Mar 15, 2022
View reviewed changes
.github/workflows/preDeploy.yml Outdated Show resolved Hide resolved
@mountiny @cead22
Update .github/workflows/preDeploy.yml
86ef870 
Co-authored-by: Carlos Alvarez <carlos@expensify.com>
@mountiny mountiny requested a review from cead22 March 15, 2022 15:09
@mountiny
Copy link
Contributor Author

@cead22 Thanks! Asking for a review again:)

cead22
cead22 previously approved these changes Mar 15, 2022
View reviewed changes
roryabraham
roryabraham requested changes Mar 15, 2022
View reviewed changes
.github/workflows/preDeploy.yml Outdated
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.

@mountiny mountiny requested review from roryabraham and cead22 March 16, 2022 18:57
@mountiny
Copy link
Contributor Author

@roryabraham Thank you for the review! Updated the workflow to be dependent directly on the getMergedPullRequest job!

@roryabraham roryabraham merged commit ca9695b into main Mar 16, 2022
@roryabraham roryabraham deleted the vit-fixIsExpensifyEmployee branch March 16, 2022 21:18
@roryabraham
Copy link
Contributor

This broke shit: https://github.com/Expensify/App/actions/runs/1995172499

@roryabraham roryabraham mentioned this pull request Mar 16, 2022
Merged
mountiny
mountiny commented Mar 17, 2022
View reviewed changes
.github/workflows/preDeploy.yml
Comment on lines +52 to +57
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 }}
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

@OSBotify OSBotify mentioned this pull request Mar 17, 2022
Closed
31 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @roryabraham in version: 1.1.44-0 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roryabraham roryabraham roryabraham approved these changes

@cead22 cead22 Awaiting requested review from cead22

Assignees

@mountiny mountiny

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mountiny @roryabraham @OSBotify @cead22