Skip to content
This repository has been archived by the owner on Nov 6, 2023. It is now read-only.

feat: GitHub Action to manage Review Apps on Heroku #1

Merged
merged 4 commits into from
May 11, 2022
Merged

feat: GitHub Action to manage Review Apps on Heroku #1

merged 4 commits into from
May 11, 2022

Conversation

RyanGWU82
Copy link
Contributor

@RyanGWU82 RyanGWU82 commented May 10, 2022

This is a GitHub Action that can be used as an alternative to Heroku Review Apps! It manages the full lifecycle of a review app:

  • When a Pull Request is opened or reopened, a review app is created in the given Heroku pipeline
  • When a Pull Request is synchronized, the review app is rebuilt with the latest source
  • When a Pull Request is closed, the review app is deleted

There's more documentation in the README.md file, so I'd start there.

I created this by starting with the review app tool I had built for readme (https://github.com/readmeio/readme/pull/6616) and modifying it to work as a GitHub action. As a result most of the code in src/heroku.js, src/netrc.js, and src/git.js is copied over from that script, along with the tests for those files.

Note that the node_modules directory needs to be checked into this repo, so that the GitHub Action can run without a separate install step. I've committed that directly to main so that it doesn't show up in this PR.

Also note that this repo is public so please don't post anything proprietary in your PR comments!

}
return result;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-05-10 at 2 32 04 PM

});
return postComment(comment);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-05-10 at 2 32 18 PM

});
return postComment(comment);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-05-10 at 2 32 35 PM

Copy link

Choose a reason for hiding this comment

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

Very very nice.

@RyanGWU82 RyanGWU82 marked this pull request as ready for review May 10, 2022 21:33
@kanadgupta
Copy link
Member

A couple of overarching questions before I dive into a proper review:

  1. It looks like linting + tests aren't being run, is that on your radar? Maybe we can just run those via GitHub Action?
  2. I'm generally hesitant about tracking node_modules via git, it was a big pain point with our old GitHub Action. I know that the GitHub Actions docs recommend using @vercel/ncc to build a dist and tracking that instead, have you looked into that? I've used that approach in one of my personal projects and I've been very happy with that approach!

@RyanGWU82
Copy link
Contributor Author

Hi @kanadgupta:

  1. I just added a CircleCI workflow. 😄
  2. I tried ncc before and it just doesn't seem to work at all. I tried again just now and captured the exact output, any ideas what's up here? (There's definitely a package.json in /Users/ryan/readmeio/heroku-review-app-action)
# Run ncc like they suggest
$ ncc build index.js 
-bash: ncc: command not found

# OK, run ncc with npx
$ npx ncc build index.js 
ncc: Version 0.33.4
ncc: Compiling file index.js into CJS
9kB  dist/index.js
9kB  [280ms] - ncc 0.33.4

# Hmmm, 9 KB with all these dependencies? 🤔 
# Let's try running the script anyway to see what happens...
$ node dist/index.js 
/Users/ryan/readmeio/heroku-review-app-action/dist/index.js:194
    throw new Error('Unable to find package.json in any of:\n[' + pathString + ']')
    ^

Error: Unable to find package.json in any of:
[/Users/ryan/readmeio,
/Users/ryan/readmeio/heroku-review-app-action]
    at init (/Users/ryan/readmeio/heroku-review-app-action/dist/index.js:194:11)
    at Object.814 (/Users/ryan/readmeio/heroku-review-app-action/dist/index.js:242:25)
    at __nccwpck_require__ (/Users/ryan/readmeio/heroku-review-app-action/dist/index.js:301:42)
    at /Users/ryan/readmeio/heroku-review-app-action/dist/index.js:332:1
    at /Users/ryan/readmeio/heroku-review-app-action/dist/index.js:338:3
    at Object.<anonymous> (/Users/ryan/readmeio/heroku-review-app-action/dist/index.js:341:12)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1157:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)

@RyanGWU82
Copy link
Contributor Author

Ah, the ncc issue is ilearnio/module-alias#81 because I'm using module-alias… unfortunately they've marked module-alias as a known incompatibility.

Let me see if I can get things to run properly without module-alias...

Copy link

@dok dok left a comment

Choose a reason for hiding this comment

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

LGTM

});
return postComment(comment);
};

Copy link

Choose a reason for hiding this comment

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

Very very nice.

src/heroku.js Show resolved Hide resolved
src/netrc.js Outdated Show resolved Hide resolved
@RyanGWU82
Copy link
Contributor Author

@kanadgupta OK, I've got ncc working so that we don't have to version our node_modules. Thanks for pushing back on that, this is much cleaner!

@RyanGWU82
Copy link
Contributor Author

With 1 approval I'm going to merge this so that I can start adding the action to our repos, but I'd encourage everyone to keep reviewing this and comment if you have any questions or feedback. 🙇🏼

@RyanGWU82 RyanGWU82 merged commit 583d24d into main May 11, 2022
@RyanGWU82 RyanGWU82 deleted the dev branch May 11, 2022 18:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants