Implement previews for GitHub pull requests #1835
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ekohl
commented
May 18, 2024
|
@ekohl Could you please review why one of the checks failed? And then this is good to merge? |
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.
ekohl
force-pushed
the
generate-pr-preview
branch
from
6b820be to
9a79485
Compare
|
I finally had some time to play with previews again. In ekohl#11 I tested this and mostly found why it didn't work. The only issue I still have is that However, I think this already has value for people. It does require an admin to set up a surge.sh token and I don't have those permissions. In my testing I created an account for myself but it's better if the DjangoGirls repo uses its own. |
|
Hey @ekohl, would it be possible to make you an admin and then you could setup the token? This is a bit out of my expertise. I think you can contact @amakarudze |
|
That works for me |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.