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

Feature Request: On pull requests, add a comment with a brief diff of the uploaded artefact and the one from main #168

Open
emdash-ie opened this issue Sep 13, 2024 · 1 comment

Comments

@emdash-ie
Copy link
Contributor

emdash-ie commented Sep 13, 2024

Proposal

A couple of times recently I’ve manually diffed Riff Raff artefacts from a pull request with those on the main branch, to understand exactly how the PR changes affect the final artefact. It’d be nice if actions-riff-raff could do that for me, to save me the time.

Details

Specifically, I’ve been downloading the folder from riffraff-artifact for the build on the PR and the build on main, expanding all archive files within those folders (e.g. running unzip on .jars, ar -x on .debs, tar -xzf on .tar.gzs, and tar -xf on .tars), and then diffing the resulting directory structures using diff --recursive.

For example, here’s the output from doing this on facia-tool this morning:

> diff --recursive --brief ./8383 ./8388

Files ./8383/facia-tool/control/md5sums and ./8388/facia-tool/control/md5sums differ
Files ./8383/facia-tool/data/usr/share/facia-tool/lib/META-INF/resources/webjars/facia-tool/1.0/fronts-client-v2/dist/index.js and ./8388/facia-tool/data/usr/share/facia-tool/lib/META-INF/resources/webjars/facia-tool/1.0/fronts-client-v2/dist/index.js differ
Files ./8383/facia-tool/data/usr/share/facia-tool/lib/META-INF/resources/webjars/facia-tool/1.0/fronts-client-v2/dist/index.js.map and ./8388/facia-tool/data/usr/share/facia-tool/lib/META-INF/resources/webjars/facia-tool/1.0/fronts-client-v2/dist/index.js.map differ
Files ./8383/facia-tool/data/usr/share/facia-tool/lib/facia/BuildInfo$.class and ./8388/facia-tool/data/usr/share/facia-tool/lib/facia/BuildInfo$.class differ
Files ./8383/facia-tool/data/usr/share/facia-tool/lib/public/fronts-client-v2/dist/index.js and ./8388/facia-tool/data/usr/share/facia-tool/lib/public/fronts-client-v2/dist/index.js differ
Files ./8383/facia-tool/data/usr/share/facia-tool/lib/public/fronts-client-v2/dist/index.js.map and ./8388/facia-tool/data/usr/share/facia-tool/lib/public/fronts-client-v2/dist/index.js.map differ

In this case --brief (which prevents outputting a diff) is important because index.js produces a huge and ugly diff: otherwise I normally wouldn’t use it, as the diff is useful for seeing how the two md5sums files differ above, for example.

Potential Improvements

  • Expand archive files like I have above
    • Without this the diff might still be useful when changing the artefact directory structure, but probably not generally for other PRs
  • For compiled files (e.g. .class files), run them through a decompiler to produce a nice diff
  • Output a diff for files where the diff is small/pleasant/not enormous

Possible Approach

I’ve been thinking about how to implement this: one approach is to download and check manifests from riffraff-builds in descending order of build number until one is for the main branch, then download that artefact. This would mean a lot more S3 calls though, so it might slow actions-riff-raff down a lot.

Potential Problems

  • If the current branch is behind main, the diff might be confusing
@emdash-ie
Copy link
Contributor Author

For an alternative approach, I guess actions-riff-raff could store a github actions artifact that just lists the most recent build on main? Then runs on pull requests could load it to know what main build to compare to.

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

No branches or pull requests

1 participant