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

Implement previews for GitHub pull requests #1835

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented May 18, 2024

When a contributor submits a PR, we always perform a build. This takes it a step further and uploads that a custom surge.sh domain. It adds a sticky comment to link to that preview while also generating some diffs. This means reviews easier.

In the implementation an additional preview step is added. This first builds the base (target of the PR) as the current. Then it downloads the generated preview that was added as an artifact in the original build step. Creating a reasonably sized diff was tricky, because there's a long Javascript line that includes the mtime, making it indeterministic. That line isn't relevant anyway, so it's removed. The diff command also ignores the search index.

All of that is placed in the preview, making it readable. A sticky comment is added with a summary, making it easy to use. The sticky comment is updated for every push, rather than added a comment for every push. This keeps the PR conversation usable.

This work is based on what's in https://github.com/theforeman/foreman-documentation where I found previews and generated diffs extremely useful in reviewing things. We'll need to set up a surge.sh account and store the token as a secret.

I've done testing in ekohl#9 but I may still have missed some parts. For example, that was fro, the repo itself while this is a PR from a fork.

Another aspect is that it would be nice to reuse the tarball that was built to generate the website itself. From my research I found that https://github.com/marketplace/actions/download-workflow-artifact exists to download artifacts from another workflow. We'll need to increase the time to delete since it defaults to 1 day, which makes the cache rather inefficient. I also don't know if you can download artifacts from another repository (like with a PR). That's why it's now generating it inside the workflow itself.

I chose to not run the current website build in a separate workflow to avoid storing an additional artifact, but in theory we can generate both current and preview in parallel and then only do the diff in the preview step.

When a contributor submits a PR, we always perform a build. This takes
it a step further and uploads that a custom surge.sh domain. It adds a
sticky comment to link to that preview while also generating some diffs.
This means reviews easier.

In the implementation an additional preview step is added. This first
builds the base (target of the PR) as the current. Then it downloads the
generated preview that was added as an artifact in the original build
step. Creating a reasonably sized diff was tricky, because there's a
long Javascript line that includes the mtime, making it indeterministic.
That line isn't relevant anyway, so it's removed. The diff command also
ignores the search index.

All of that is placed in the preview, making it readable. A sticky
comment is added with a summary, making it easy to use. The sticky
comment is updated for every push, rather than added a comment for every
push. This keeps the PR conversation usable.
fi

- name: Deploy to surge.sh
run: npx surge ./preview $PREVIEW_DOMAIN --token ${{ secrets.SURGE_TOKEN }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just realized: this doesn't have access to the token since it runs from the PR context. I think that's why in foreman-documentation it runs after deploy` finished:
https://github.com/theforeman/foreman-documentation/blob/16ca70658a09af2b0358bd7c782edf2fc5327e3c/.github/workflows/preview.yml#L3-L8
Note it also has some more code to run if it failed vs passing. That kind of error message isn't posted now.

This needs a bit more work.

Copy link

Choose a reason for hiding this comment

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

You can use pull_request_target trigger, which has access to secrets. But then the whole workflow would need to run as that.
In foreman we do this two-stage approach which I also don't like, but the result is similar.

@fltfx
Copy link
Member

fltfx commented Oct 13, 2024

@ekohl Could you please review why one of the checks failed? And then this is good to merge?

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.

3 participants