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.

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

Remove build folder and use Netlify deploys for previews #621

Merged
merged 3 commits into from
Mar 15, 2022

Conversation

howard-e
Copy link
Contributor

@howard-e howard-e commented Feb 15, 2022

There has been some discussion around using Netlify to present PR previews (and by extension, serve as the published site because the repo's current site link depends on the build folder, which we are now proposing to remove).

Previous discussion can be seen at:

This PR introduces Netlify's use by:

  1. untracking the build folder,
  2. including netlify.toml with the directives required to display the site's content through Netlify,
  3. updating the site preview link used in PRs to point to the generated netlify deploy link, and
  4. ensuring the site's build folder can be created consistently without depending on a pre-existing build folder as reported in npm run build doesn't create build directory, fails if it doesn't exist #588 and fixed in Create build folder if it doesn't exist when running create-example-tests.js #606.

Assumptions made here:

  • The site's name will be aria-at, and so the site's url will be https://aria-at.netlify.app.
  • Netlify 'Builds' are active.
  • Netlify 'Deploy Previews' feature is enabled.
  • Netlify 'Drawer' feature is disabled.

Changes as a result of this:

  • There should be no change to how contributors currently work.
  • Generate and Commit Files workflow will need to be disabled.
  • This should hopefully make reviews a little easier to see what changed without having to filter through generated files.
  • This should hopefully be more reliable than raw.githack.com's URLs (we have recently been receiving 429: Too Many Requests errors when viewing PR Previews)
  • No build folder merge conflicts.

NOTE

@howard-e
Copy link
Contributor Author

Note that I manually cancelled the Generate and Commit Files workflow as it would be unnecessary here and the Update Pull Request with Preview Link isn't able to run from a forked repository, to explain the build failures.

@jscholes
Copy link
Contributor

@howard-e Workflow question: currently, the most common way we check that a build can be successfully performed is to just push up to GitHub, and let the action take care of it. Will that still be possible with the proposal outlined here? I.e. will the status of the build still be reported on the relevant PR?

Copy link

@jesdaigle jesdaigle left a comment

Choose a reason for hiding this comment

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

This is amazing. Thank you @howard-e !

@howard-e
Copy link
Contributor Author

@howard-e Workflow question: currently, the most common way we check that a build can be successfully performed is to just push up to GitHub, and let the action take care of it. Will that still be possible with the proposal outlined here? I.e. will the status of the build still be reported on the relevant PR?

@jscholes Instead of the Generate and commit files workflow that I think we're speaking of, we could add a workflow which runs --validate instead. I believe that's currently only being used for local development? That would still report if the build can be successfully done and we could also benefit from running that in verbose mode.

Additionally, the Netlify bot could update a comment in the PR with the status around the actual site deploy.

@jscholes
Copy link
Contributor

@howard-e I'm open to whatever. If we should just validate the tests locally instead, that's fine as a workflow change. Might actually be easier.

@jesdaigle
Copy link

@mcking65 @jscholes Merging this ticket is dependent on having a Netlify account that has the right credentials to this repo. It's currently using Howard's personal account.

@howard-e howard-e force-pushed the remove-build-folder branch 2 times, most recently from 387be59 to 6408cc8 Compare March 9, 2022 22:33
@howard-e howard-e force-pushed the remove-build-folder branch from 6408cc8 to 0fa9641 Compare March 10, 2022 18:14
@netlify
Copy link

netlify bot commented Mar 14, 2022

✔️ Deploy Preview for aria-at ready!

🔨 Explore the source changes: 9342382

🔍 Inspect the deploy log: https://app.netlify.com/sites/aria-at/deploys/622f865fa88488000837a212

😎 Browse the preview: https://deploy-preview-621--aria-at.netlify.app

@nschonni
Copy link
Contributor

#592 might resolve the need to pin to Node 12, since it's the native build that appears to be failing on a few of the other PRs that recently tried to run Netlify

@howard-e
Copy link
Contributor Author

howard-e commented Mar 15, 2022

#592 might resolve the need to pin to Node 12, since it's the native build that appears to be failing on a few of the other PRs that recently tried to run Netlify

@nschonni yep, it might. This looks good to go now so let's land this one and test with that after.

Wanted to also maintain parity with the node version that's been pinned in the workflows but nodegit may have been the reason why it was pinned as well.

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