Skip to content

Setup Onyx repository actions#4

Merged
AndrewGable merged 19 commits intomasterfrom
jules-setupGithubActions
Nov 17, 2020
Merged

Setup Onyx repository actions#4
AndrewGable merged 19 commits intomasterfrom
jules-setupGithubActions

Conversation

@Julesssss
Copy link
Contributor

@Julesssss Julesssss commented Nov 12, 2020

CC @tgolen @AndrewGable

Onyx is now a standalone package, we need to setup automated actions for versioning and linting.

Fixed Issues

Fixes: https://github.com/Expensify/Expensify/issues/145386

Tests

Added two commits to test the lint script

Pre-merge

Post-Merge

  • After merging this PR, a 'Create new version' Action should be triggered, which should bump the version to x.x.x, push the tag and auto-merge the version bump PR
  • If this does not occur, or fails, please re-open the issue

@Julesssss Julesssss self-assigned this Nov 12, 2020
@Julesssss Julesssss marked this pull request as ready for review November 12, 2020 17:55
@Julesssss Julesssss changed the title [WIP] Setup Onyx repository actions Setup Onyx repository actions Nov 13, 2020
@Julesssss Julesssss force-pushed the jules-setupGithubActions branch from de8bc51 to b14e13b Compare November 13, 2020 14:27
@Julesssss Julesssss force-pushed the jules-setupGithubActions branch from b14e13b to b7a1179 Compare November 13, 2020 14:29
@Julesssss Julesssss force-pushed the jules-setupGithubActions branch 2 times, most recently from f66a6f4 to acdf2a2 Compare November 13, 2020 14:45
@Julesssss Julesssss force-pushed the jules-setupGithubActions branch from acdf2a2 to e4a7bd2 Compare November 13, 2020 14:46
@Julesssss
Copy link
Contributor Author

Julesssss commented Nov 16, 2020

  • Added the NPM Lint Action
    • Works as expected
  • Added additional logic to the version Action
    • Version generation works as expected, but needs to be changed has been changed to basic semver for versioning
    • Blocked by a bad BOTIFY_TOKEN auth token, hoping to test out an alternate key sometime today

@Julesssss Julesssss requested review from a team, AndrewGable and tgolen November 16, 2020 16:59
@botify botify requested review from sketchydroide and removed request for a team November 16, 2020 16:59
run: git checkout -b version-bump-${{ github.sha }}

- name: Generate version
run: npm version patch -m "Update version to %s"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

'npm version patch' is the only change from our existing ReactNativeChat Actions

Copy link
Contributor

@AndrewGable AndrewGable left a comment

Choose a reason for hiding this comment

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

Looks great!

with:
ssh-private-key: ${{ secrets.SSH_PRIVATE_KEY }}

- name: Install dependenices
Copy link
Contributor

Choose a reason for hiding this comment

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

dependenices typo ?

Copy link
Contributor

@sketchydroide sketchydroide left a comment

Choose a reason for hiding this comment

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

Looks great, just a typo found I think

Copy link
Contributor

@sketchydroide sketchydroide left a comment

Choose a reason for hiding this comment

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

LGTM

@tgolen
Copy link
Collaborator

tgolen commented Nov 17, 2020

Would you mind adding two commits to ensure that the linting is working?

  1. First commit - introduce a linting error in one file
  2. Second commit - fixing the linting error

@Julesssss
Copy link
Contributor Author

@Julesssss
Copy link
Contributor Author

Thanks for reviews and test suggestions. Please see above for example of a failing lint Action

@tgolen
Copy link
Collaborator

tgolen commented Nov 17, 2020

Awesome, thanks!

@AndrewGable AndrewGable merged commit 46d4b38 into master Nov 17, 2020
@AndrewGable AndrewGable deleted the jules-setupGithubActions branch November 17, 2020 18:08
@AndrewGable
Copy link
Contributor

As soon as I merged this I realized we were probably forgetting one workflow: https://github.com/Expensify/ReactNativeChat/blob/master/.github/workflows/automerge.yml

The version action will create the PR here: #6

But it won't automatically merge it until we have the automerge action in place.

@AndrewGable
Copy link
Contributor

Also.. We might want to beef up the security checks on https://github.com/Expensify/ReactNativeChat/blob/master/.github/workflows/automerge.yml - As I believe right now any PR that is labeled "auto merge" will be merged automatically, which works for internal use, but might be a bit riskier for open source usage.

@tgolen
Copy link
Collaborator

tgolen commented Nov 17, 2020 via email

@Julesssss
Copy link
Contributor Author

Julesssss commented Nov 18, 2020

Thanks for adding the automerge Action @AndrewGable

@Julesssss
Copy link
Contributor Author

On that note, I raised a point regarding Github Action security yesterday in Infra -- It looks like we'll need to block Actions from running on forked PRs.

@AndrewGable
Copy link
Contributor

If they don't run on forked PRs, how do we verify they are linted correctly?

@Julesssss
Copy link
Contributor Author

how do we verify they are linted correctly?

Exactly -- this is obviously not going to be acceptable, so we will need to verify the security concern or look into alternatives. The initial concern was that anyone can create Actions that run against the repo, and in theory would be able to expose our repo secrets.

@Julesssss
Copy link
Contributor Author

Opened an issue to collect and thoughts regarding this: https://github.com/Expensify/Expensify/issues/146339

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants