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

Create isStagingDeployLocked action #1683

Merged
merged 45 commits into from
Mar 12, 2021
Merged

Create isStagingDeployLocked action #1683

merged 45 commits into from
Mar 12, 2021

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Mar 9, 2021 •
edited
Loading

Details

Creates an action to check if the StagingDeployCash is locked.

Fixed Issues

Fixes (partial) https://github.com/Expensify/Expensify/issues/155140

Tests

Tested On

Github

@roryabraham roryabraham requested a review from AndrewGable March 9, 2021 18:41
@roryabraham roryabraham requested a review from a team as a code owner March 9, 2021 18:41
@roryabraham roryabraham self-assigned this Mar 9, 2021
@botify botify requested review from Julesssss and removed request for a team March 9, 2021 18:42
@roryabraham
Copy link
Contributor Author

Okay, this isn't working because I (foolishly!) forgot that we can't use ES6 modules in a Node.js script using the CJS module system. I knew that I was going to forget and this was going to happen...

Anyways, I'm going to put together a PR to port over the ES6 GithubUtils and tests from expensify-common to Expensify.cash/.github/libs/ and convert it to use CJS modules.

@roryabraham roryabraham changed the title Create isStagingDeployLocked action [HOLD] Create isStagingDeployLocked action Mar 9, 2021
@roryabraham
Copy link
Contributor Author

This needs to be HELD on #1690

@roryabraham
Copy link
Contributor Author

Okay, this is well-tested and ready for review

@roryabraham roryabraham changed the title [HOLD] Create isStagingDeployLocked action Create isStagingDeployLocked action Mar 11, 2021
AndrewGable
AndrewGable reviewed Mar 11, 2021
View reviewed changes
.github/actions/isStagingDeployLocked/isStagingDeployLocked.js
const githubUtils = new GithubUtils(octokit);

githubUtils.getStagingDeployCash()
.then(({labels}) => core.setOutput('IS_LOCKED', _.contains(_.pluck(labels, 'name'), '🔐 LockCashDeploys 🔐')))
Copy link
Contributor

Choose a reason for hiding this comment

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

Not totally required to switch, but why make this it's own action when we could use something already built like: https://github.com/marketplace/actions/github-api-request ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah is it because this needs getStagingDeployCash()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's just to take advantage of the code we've already built for fetching the open StagingDeployCash

@Julesssss
Copy link
Contributor

Tests seem good.

But I'm confused by isStagingDeployLocked/index.js. Why do we need this full compiled code included here in the action? Is this simply the only way to depend on GithubAction functions? Would the alternative to publish the class as a library?

@roryabraham
Copy link
Contributor Author

@Julesssss from the GH Actions docs:

GitHub downloads each action run in a workflow during runtime and executes it as a complete package of code before you can use workflow commands like run to interact with the runner machine. This means you must include any package dependencies required to run the JavaScript code. You'll need to check in the toolkit core and github packages to your action's repository.

So we had a few options:

  1. Commit node_modules ❌
  2. Put each action in a separate repo, and commit the node modules in each repo.
  3. Use ncc to precompile/bundle each action with its dependencies.

We've opted for (3)

@AndrewGable
Copy link
Contributor

FYI conflicts

@roryabraham
Merge branch 'master' into Rory-isStagingDeployLocked
2ba290d 
# Conflicts:
#	tests/unit/GithubUtilsTest.js
@roryabraham
Copy link
Contributor Author

Merge conflicts resolved!

@Julesssss
Copy link
Contributor

We've opted for (3)

Thanks for explaining! Tough choice, no option is great 😞

@Julesssss
Copy link
Contributor

LGTM. Will let Andrew approve and merge.

@AndrewGable AndrewGable merged commit 0a46e36 into master Mar 12, 2021
@AndrewGable AndrewGable deleted the Rory-isStagingDeployLocked branch March 12, 2021 17:10
@github-actions github-actions bot locked and limited conversation to collaborators Mar 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Julesssss Julesssss Julesssss approved these changes

@AndrewGable AndrewGable AndrewGable 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.
3 participants
@roryabraham @Julesssss @AndrewGable