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

[No QA] Verify that commits are signed when PRs are created or synchronized #7981

Merged
merged 8 commits into from
Mar 9, 2022
Merged

[No QA] Verify that commits are signed when PRs are created or synchronized #7981

merged 8 commits into from
Mar 9, 2022

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Mar 2, 2022 •
edited by neil-marcellini
Loading

Details

This PR adds a new PR check to verify that commits are signed.

Fixed Issues

https://expensify.slack.com/archives/C01GTK53T8Q/p1646185784242179

Tests

  1. All the commits on this PR were originally unsigned:

    image

  2. Then I got a workflow run where those commits were verified to be unsigned.

    https://github.com/Expensify/App/runs/5395802072?check_suite_focus=true

  3. Then I retroactively signed the commits on this PR:

    image

  4. Then the check passed 🎉

    https://github.com/Expensify/App/runs/5395944872?check_suite_focus=true

  5. After merging this, we also need to make sure that these tests pass when the PR is coming from a fork.

  • Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

  • I made sure to pull main before submitting my PR for review
  • 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 ran the tests & they passed on all platforms
  • I included screenshots or videos for tests on all platforms
  • 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 guidelines as stated in the Review Guidelines

PR Reviewer Checklist

  • I verified the Author pulled main before submitting the PR
  • 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 verified tests pass on all platforms & I tested again on all platforms
  • I checked that screenshots or videos are included for tests on all platforms
  • 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 that this PR follows the guidelines as stated in the Review Guidelines

Tested On

GitHub only

@roryabraham roryabraham requested a review from a team as a code owner March 2, 2022 16:51
@MelvinBot MelvinBot requested review from neil-marcellini and removed request for a team March 2, 2022 16:51
@neil-marcellini
Copy link
Contributor

This is a work in progress correct?

@roryabraham
Copy link
Contributor Author

Yes, sorry I should've marked it as so @neil-marcellini

@roryabraham roryabraham changed the title Test unsigned commit check [WIP] Test unsigned commit check Mar 2, 2022
@roryabraham roryabraham changed the title [WIP] Test unsigned commit check [WIP] [No QA] Test unsigned commit check Mar 2, 2022
@roryabraham roryabraham self-assigned this Mar 2, 2022
@roryabraham roryabraham changed the title [WIP] [No QA] Test unsigned commit check [No QA] Test unsigned commit check Mar 2, 2022
@roryabraham
Copy link
Contributor Author

Ready for review

@roryabraham roryabraham changed the title [No QA] Test unsigned commit check [No QA] Verify that commits are signed when PRs are created or synchronized Mar 2, 2022
neil-marcellini
neil-marcellini reviewed Mar 2, 2022
View reviewed changes
Copy link
Contributor

@neil-marcellini neil-marcellini left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I'm not too familiar with GH actions, so I'll let another person approve.

.github/actions/verifySignedCommits/verifySignedCommits.js Outdated
const allCommitsSigned = _.every(data, datum => datum.commit.verification.verified);

if (!allCommitsSigned) {
const unsignedCommits = _.filter(data, datum => !datum.commit.verification.verified);
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: Instead of having a separate check of allCommitsSigned you could replace it with this
const unsignedCommits = _.filter(data, datum => !datum.commit.verification.verified);
And then in the check:
if (!_.isEmpty(unsignedCommits)) {

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @neil-marcellini and think it's NNAB (NOT not a blocker 😅)

deetergp
deetergp requested changes Mar 8, 2022
View reviewed changes
.github/actions/verifySignedCommits/verifySignedCommits.js Outdated
const allCommitsSigned = _.every(data, datum => datum.commit.verification.verified);

if (!allCommitsSigned) {
const unsignedCommits = _.filter(data, datum => !datum.commit.verification.verified);
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @neil-marcellini and think it's NNAB (NOT not a blocker 😅)

@roryabraham
Copy link
Contributor Author

Updated and ready for another review

neil-marcellini
neil-marcellini approved these changes Mar 8, 2022
View reviewed changes
Copy link
Contributor

@neil-marcellini neil-marcellini left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, it looks good to me.

deetergp
deetergp approved these changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@deetergp deetergp left a comment

Choose a reason for hiding this comment

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

LGTM too 👍

@deetergp deetergp merged commit bc4f429 into main Mar 9, 2022
@deetergp deetergp deleted the Rory-VerifiedCommitsCheck branch March 9, 2022 02:30
@OSBotify
Copy link
Contributor

OSBotify commented Mar 9, 2022

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

@OSBotify OSBotify mentioned this pull request Mar 10, 2022
Closed
@OSBotify
Copy link
Contributor

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

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

@OSBotify
Copy link
Contributor

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

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

@deetergp deetergp deetergp approved these changes

@neil-marcellini neil-marcellini neil-marcellini approved these changes

Assignees

@roryabraham roryabraham

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@roryabraham @neil-marcellini @OSBotify @deetergp