devicon icon indicating copy to clipboard operation
devicon copied to clipboard

Add optimize_icons workflow triggered on push

Open ConX opened this issue 2 years ago • 4 comments

Double-check these details before you open a PR

  • [x] PR does not match another non-stale PR currently opened

Features

This PR closes #1715

Notes

This PR replaces the now closed #1718.

In the previous version, we were trying to make it run on the PR itself, but that doesn't seem to be possible, given the following:

This version simply runs the optimizer after the PR merge (on the corresponding push). I have tested this with a second fork of the repository, and it seems to work as expected. According to the documentation) when SVGO is invoked on push:

In the push context the SVGO Action will optimize all SVGs that have been added or modified in the commit(s) being pushed. Any SVGs that are in the repository but have not been modified in the commit(s) will not be optimized.

Doing this on the push has the disadvantage of not allowing us to review the optimized versions, but it will ensure all icons in develop are optimized.

ConX avatar May 17 '23 06:05 ConX

Not sure if the commit will be able to be pushed as the develop branch is protected. Would a PR towards the develop branch work in this case? 🤔

Panquesito7 avatar May 18 '23 18:05 Panquesito7

Not sure if the commit will be able to be pushed as the develop branch is protected. Would a PR towards the develop branch work in this case? thinking

I think branch restrictions can have exceptions for certain users. Maybe we can give the bot or github tokdn permission to push directly? Not sure if that's something we'd want though. I'd prefer if it was done before we merge the PR instead.

@ConX the 2nd issue should be fixable by making a preflight script similar to what we do with the in-develop-labeler script. As for the first issue... Are there any of the other triggers that might work?

Snailedlt avatar May 23 '23 17:05 Snailedlt

Probably we should configure the action to create a PR towards the develop branch only. Granting access to GitHub Actions isn't really safe. If someone modifies the actions in a PR, they could end up doing unwanted changes to the repository, AFAIK.

Panquesito7 avatar May 23 '23 17:05 Panquesito7

Granting access to GitHub Actions isn't really safe.

ACK ;)

If someone modifies the actions in a PR, they could end up doing unwanted changes to the repository, AFAIK.

Well, the action modification must pass a review and merge first. Changing the action in a PR runs with the actions of the current (not the PR'ed) version. Hmm, is that easy to understand? A PR runs with the current actions, even if it wants to introduce changes with the actions. The changed actions are only used after they have been pulled.

In my opinion that makes it ok-ish to have actions with write permissions, just everyone who can merge must know this and watch out for unwanted changes :grimacing:

Finii avatar Sep 05 '24 08:09 Finii