ci: refactor container size diff action to use github-script #908
Conversation
Hi 👋 I came across your composite action and found it useful so I decided to re-write it using github action script instead. Free free to discard this PR if you are happy with bash but I thought I should share. Thanks! Signed-off-by: Devin Buhl <onedr0p@users.noreply.github.com>
Signed-off-by: Devin Buhl <onedr0p@users.noreply.github.com>
Signed-off-by: Devin Buhl <onedr0p@users.noreply.github.com>
onedr0p
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Hi @onedr0p! Thanks for your contribution. Great to see you have found the size comparison action useful. Can you take a look at the feedback from CoPilot and the Zizmor findings? I don’t see any objections to merging this when those are fixed. |
Signed-off-by: Devin Buhl <onedr0p@users.noreply.github.com>
|
I believe that should do it! |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the container size diff GitHub action from a bash script implementation to use the github-script action with Node.js. The change maintains the same functionality while improving code structure and readability.
Key changes:
- Replaces bash script execution with github-script action
- Implements container size comparison logic directly in JavaScript
- Adds proper error handling and platform filtering
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
I noticed you are still making changes to the action in your context (home-operations/containers#783). Shall I wait with merging this? As I like the direction of your PR. |
|
@rjaegers Yeah sure, I'll update it here when I am done testing. Just adding in some npm modules to try and improve readability at the cost of some external modules. I will mark this as a draft for now and switch it back to review when done. |
Signed-off-by: Devin Buhl <onedr0p@users.noreply.github.com>
|
I updated this and added caching of npm packages. The message now looks like this: 
|
Signed-off-by: Devin Buhl <onedr0p@users.noreply.github.com>
|
|
Looks like you are running into the same issue as me over in my repo 😅 https://www.github.com/home-operations/containers/issues/792 |
Indeed, and its a funny thing as this should hamper a lot of public repositories with a large(r) set of ci/cd checks. This is not the first time I run into this problem and thus far have found no nice solution. I even asked CoPilot for advice, and it insisted an option exists to re-run the workflow with elevated permissions if it comes from a fork. Although that is nowhere to be found in the GitHub documentation 😕. |
|
My workaround is to pull the PR from an external contributor and build/test it locally. Another option is to duplicate the PR so checks happen and then close it when successful, then merge the external contributors PR. |
|
🎉 Hooray! The changes in this pull request went live with the release of v6.4.0 🎉 |
|
🎉 Hooray! The changes in this pull request went live with the release of v6.4.1 🎉 |
2 participants
🚀 Hey, I have created a Pull Request
Description of changes
Hi 👋
I came across your composite action and found it useful so I decided to re-write it using github action script instead. Free free to discard this PR if you are happy with bash but I thought I should share.
You can see a preview here home-operations/containers#770
Thanks!
✔️ Checklist