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

Add automatic deployment preview of PRs when changing the site #658

Closed
wants to merge 1 commit into from

Conversation

jkremser
Copy link
Member

@jkremser jkremser commented Oct 15, 2021

This PR should go in only after #657

Add config for netlify that will trigger the build and deploys the preview of pull-request.

Such PR needs to be opened against master and some of the following files need to be changed: {README,CONTRIBUTING,CHANGELOG}.md docs/

Example:

Some limitations:

  • It's a one shot thing so even if new commits go to the PR or if the pr is closed and opened it wont re-trigger the build.

  • We have 300mins of free build time each month and I've used 20 during the development (1 build is like a 1 or 2 minutes) it should be more than enough

Also the website required GH token to be able to fill some metadata about the repo - JEKYLL_GITHUB_TOKEN. Fortunately, it uses the thing only in read only mode probably to prevent the rate limiter blocks. I've set this token in the Netlify web and used one that doesn't have write access to this repo w/ the minimal privileges. It's a token from my wife actually who has access to 1 private repo w/ her web site :D So even if it leaks, no biggie.

If it works well, some future improvement could be running this thing also if the PR is opened against gh-pages (change to layouts, CSS, etc.)

Signed-off-by: Jirka Kremser [email protected]

@somaritane
Copy link
Contributor

Some limitations:

  • It's a one shot thing so even if new commits go to the PR or if the pr is closed and opened it wont re-trigger the build.

That means we won't be able to correct the discovered layout issues within the same PR, so the proposed solution doesn't address the need for constant fast feedback during authoring.
The author will have to check k8gb.io layout locally anyway before PR publishing, which kinda defies the purpose :)

Also the website required GH token to be able to fill some metadata about the repo - JEKYLL_GITHUB_TOKEN. Fortunately, it uses the thing only in read only mode probably to prevent the rate limiter blocks. I've set this token in the Netlify web and used one that doesn't have write access to this repo w/ the minimal privileges. It's a token from my wife actually who has access to 1 private repo w/ her web site :D So even if it leaks, no biggie.

This token is a standard GitHub token with a public_repository scope, it can be created by any project maintainer.
https://jekyll.github.io/github-metadata/authentication/
So I'd rather suggest having the token created by one of the project maintainers or for a connected robot account in order to preserve continuity.
The same goes for the Netlify account. My understanding is that it is being registered to email. Is it possible to add other project members to it?

Also, my understanding is Netlify acts like a bot, as one of the PR checks. What happens if the Netlify account is deleted, locked, or if the available monthly build minutes threshold is exceeded? Will it be possible to still proceed with PR merge?

@jkremser
Copy link
Member Author

The author will have to check k8gb.io layout locally anyway before PR publishing, which kinda defies the purpose :)

It's not perfect, but that's kind of how I would use it anyway. Working on web related change locally, verifying it on the gh-pages branch and only when it looks ok, then open the pr where the preview is deployed and easier for other to see the change. That's how they PR preview feature work ootb, they have also REST api that can start a build and deploy it, say, from github action, github comments are also doable, but I'd start simple. Also if the comments are done naively, it will spam on each change which is not ideal either, on the other hand if the previous bot's comment is deleted and new one is added (I think that's what dependabot does), the previous preview are lost.. One shot approach is very simple and definitely better than nothing.

wrt JEKYLL_GITHUB_TOKEN

imho a token from one of the maintainers can't/should not be used here. The reason is that GH doesn't provide a way to control the RBAC in a per repo base (it's not granular enough, + doesn't support read only on repo). So if I used mine or your token w/ public_repo scope, it would mean that if the token leaks, the attacker would have the write access to all the repos where the user impersonated by the token has. If we have a token for a robot that doesn't have the write permission to our repo, I'd change that; otherwise I'd rather use some dummy token, because we shouldn't trust the Netlify that it won't leak (actually it's not encrypted there).

The same goes for the Netlify account. My understanding is that it is being registered to email. Is it possible to add other project members to it?

Definitely, bus-factor == 1 is always bad, I'll invite you all once it's merged (it can't be part of the PR :D)

What happens if the Netlify account is deleted ... Will it be possible to still proceed with PR merge?

It's an optional thing running externally in async fashion. If their servers are down, the limit was reached or anything bad happened, then it just wont put that comment on the PR that's all.

@@ -0,0 +1,7 @@
[build]
command = "git config remote.origin.url || git remote add -f -t gh-pages origin https://github.com/k8gb-io/k8gb && git fetch origin gh-pages:gh-pages && git checkout gh-pages && make netlify-build"
Copy link
Member

Choose a reason for hiding this comment

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

command is too long and complex. Can be moved to small sh script?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can, I was thinking about it, but I didn't want to pollute the / of the repo with netlify specific stuff. I can change that if you think, it's better. Well, it will be more readable definitely. In that case we probably don't need the Makefile target on gh-pages branch at all. The script can be quite dangerous, you know changing branches, doing checkouts, adding remotes, but I can also add the big if $CI=true conditions, or similar flag that Netlify has

Copy link
Member Author

Choose a reason for hiding this comment

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

build-web.sh / netlify.sh / .netlify.sh ? I like the last one..

Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this at all? does netlify erase git metadata?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't get your question @k0da :) The command is not about git metadata, it's about telling netlify how to build the website. The remote called origin is not set in their environment (no idea how they clone the repo, it's not part of the logs) so we need to add it, then fetching also the gh-pages branch (again not fetched by them), then basically simulating this: https://github.com/k8gb-io/k8gb/blob/master/.github/workflows/gh-pages.yaml#L21:L24 (part is done here in this one liner and the other part on the gh-pages' Makefile -> not ideal, merging it to 1 bash script)

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