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

Adds image compression action on pull requests #14

Closed
wants to merge 1 commit into from

Conversation

claudenirmf
Copy link
Member

Adds image compression action. This action is to be triggered on pull requests.

NOTICE: there is a discussion on this action's repo about it not working in pull requests coming from forked repos (see this issue discussion). The developers have disabled the action in these cases, which may require an additional PR on the original repo to trigger the action and actually compress the image files. The disabling of the action avoids silent error on the action's log.

I have submitted a question to the developers for more clarification, but we could merge this PR in the meantime.

Closes #11.

@claudenirmf
Copy link
Member Author

claudenirmf commented Nov 15, 2021

Considering these instructions, I would consider having some staging or development branch from which we merge changes into master. Having a separated master branch should allow us running the action internally and may make it easier for us to version the repository through releases.

I would not consider creating a GitHub token with elevated permissions because this may introduce unnecessary vulnerabilities, e.g., from dependabot-like PRs.

@tgoprince
Copy link
Member

tgoprince commented Nov 16, 2021

I support your staging branch solution.

Could you just clarify how often do you envision merging the staging branch with the master branch?
I don't see a need to version this dataset, so I'm assuming it would follow every PR from a forked repository.

We could automate the creation of this internal PR with something like this:

https://github.com/marketplace/actions/create-pull-request

@tgoprince
Copy link
Member

I tested this on action on #15.

It provides some nice feedback on the PR and it parses all images in the repository, but only commits those that have actually been compressed.

Overall, this is a very good script for our dataset.

Good job @claudenirmf!

@claudenirmf
Copy link
Member Author

Maybe we won't need to "version" the repo, but creating releases may be necessary to ease the reproducibility of published experiments. Zenodo could also be used here.

For the staging branch, I would expect a new PR from it into the master for every PR coming from a forked repo. I like your idea of automating this staging/pr step but I don't know if we could have problems if two PRs were to be created in a very short period of time. The second automated PR could interfere with the first, I don't know.

I will try to add the action you suggested before merging this PR. Thanks for the feedback!

@tgoprince tgoprince closed this Mar 14, 2023
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.

Github action to compress images
2 participants