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

Add optimize_icons workflow triggered on push #1764

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ConX
Copy link
Contributor

@ConX ConX commented May 17, 2023

Double-check these details before you open a PR

  • 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 ConX added the devops Use this label for devops related enhancements label May 18, 2023
@Panquesito7
Copy link
Member

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? 🤔

@Snailedlt
Copy link
Collaborator

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?

@Panquesito7
Copy link
Member

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.

@ConX ConX marked this pull request as draft May 24, 2023 12:05
@Finii
Copy link

Finii commented Sep 5, 2024

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 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops Use this label for devops related enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants