Bugfix: Fix optimize_icons workflow error on PRs #1718

ConX · 2023-03-28T05:51:05Z

Double check these details before you open a PR

PR does not match another non-stale PR currently opened

Features

According to the git-auto-commit-action README, the ref should be specified in the preceding action/checkout@v3.

If successful, this PR closes #1715

Notes

The two aarch64 icon files that have been updated were to test whether the workflow succeeds.

...existing PRs.

lunatic-fox · 2023-03-28T09:39:21Z

Thank you to open this PR! 🚀
Unfortunately, it seems that still not working properly, but I'll try something else on the next commit.

lunatic-fox · 2023-03-28T11:01:09Z

I was reading the docs a little bit more and I think that's still an issue: Use in forks from public repositories. Maybe I got it wrong, I hope so. 🤔

ConX · 2023-03-28T18:33:48Z

I was reading the docs a little bit more and I think that's still an issue: Use in forks from public repositories. Maybe I got it wrong, I hope so. 🤔

I couldn't test that on my fork, but I suspect it will be an issue. The two possible solutions that I am thinking of are:

Edit the PULL_REQUEST_TEMPLATE for new icons to include a checkbox to ask the contributor to enable `Allow edits by maintainers".
Make the SVG optimization only part of the merge instead of being triggered at each new PR or PR update. The way I think this might work is to run right after a merge on our develop branch, where there shouldn't be any permission issue.

ConX · 2023-03-28T18:35:37Z

Thank you to open this PR! 🚀 Unfortunately, it seems that still not working properly, but I'll try something else on the next commit.

~~The changes here are not getting executed right now since this hasn't been merged with the develop branch yet. I think we should merge it and check with existing/failing PRs.~~

EDIT: I did a quick debug "print" test to check if indeed the updated workflow is getting triggered, and you are correct that it doesn't seem to be working properly.

…into exp-optimize-icons

ConX · 2023-03-30T05:41:26Z

Based on this this issue comment from the author of the git-auto-commit-action, I switched the listening even to pull_request_target, which should run when the PR is merged and executed on the base branch (which in our case should be develop).

Is it worth merging to see if it works?

lunatic-fox

Based on this this issue comment from the author of the git-auto-commit-action, I switched the listening even to pull_request_target, which should run when the PR is merged and executed on the base branch (which in our case should be develop).

Is it worth merging to see if it works?

I think it does worth it! 🚀
Based on your comment, the linked comment and GitHub docs, that seems exactly what we want to do, once it will run, I think with the permissions of the original develop.

Snailedlt

I think we're safe to apply the changes after merging, but are there any cases where the workflow might fail, and therefore cause issues?

I'd argue that error handling is much more important when applying changes after the merge, as opposed to before.

Also, does this workflow run when merging to madter as well, or just to develop?

lunatic-fox · 2023-03-30T09:28:34Z

Hmm @ConX, after reading a little Keeping your GitHub Actions and workflows secure, I'm wondering if the trigger is pull_request_target and develop is the target branch, this might cause the optimized icons of pr_branch to be committed to develop and that's for sure is a problem.

Also, does this workflow run when merging to madter as well, or just to develop?

@Snailedlt I think it will since the branch wasn't specified, but we need to treat the security issues first.

I'll read the article this weekend. For now I think we should try this way, with workflow_run, that was mentioned before in other comment by @Snailedlt.

Snailedlt · 2023-03-30T11:59:24Z

Yeah, I think we should go for the preflight workflow option with workflow_run.
This is how we fixed the in-develop labeler: #1410

ConX · 2023-03-31T23:58:36Z

I'll read the article this weekend. For now I think we should try this way, with workflow_run, that was mentioned before in other comment by @Snailedlt.

Thanks for pointing this out! I agree that it's not secure as is right now.

Although I think we can make this secure given that we require approvals for merges, going with the workflow_run approach seems a better option. Should we wait to implement this after the #1410 is merged, and re-use the same preflight as the triggering workflows in workflow_run?

Snailedlt · 2023-04-01T06:43:17Z

@ConX No we shouldn't wait.
The other PR won't take effect until it's merged into master, and iirc will break some stuff before it takes effect.
Therefore we're waiting until release to merge it into develop, so that we have minimal time with a workflow that foesn't work.

This PR however shouldn't break anything, so we can merge it into develop without issues :)

Panquesito7

Looks like there are merge conflicts. Please fix them.

ConX · 2023-05-03T08:58:22Z

Looks like there are merge conflicts. Please fix them.

@Panquesito7 The workflow updated here optimize_icons.yml appears to have been deleted by #1724. Should it be re-added, or should we simply close this PR?

Snailedlt · 2023-05-03T10:20:55Z

Looks like there are merge conflicts. Please fix them.

@Panquesito7 The workflow updated here optimize_icons.yml appears to have been deleted by #1724. Should it be re-added, or should we simply close this PR?

@ConX Yes, that was just a temp fix to avoid all the errors :) We should add it back ASAP after the upcoming release

Panquesito7 · 2023-05-03T17:12:34Z

Looks like there are merge conflicts. Please fix them.

@Panquesito7 The workflow updated here optimize_icons.yml appears to have been deleted by #1724. Should it be re-added, or should we simply close this PR?

I believe it should be added again, as this PR's focused on fixing the errors. 🙂

lunatic-fox · 2023-05-03T21:19:28Z

It seems that as long as we add again optimize_icons.yml to develop this branch will stop conflicting. 🙂

Panquesito7 · 2023-05-13T00:46:52Z

Re-creating the PR would be easier, though, as all PRs will have that action and fail.
EDIT: @ConX, do you have the time for this, or should we let someone else do this? 🤔

ConX · 2023-05-13T05:31:10Z

EDIT: @ConX, do you have the time for this, or should we let someone else do this? 🤔

@Panquesito7 I can work on this during the weekend. Based on @Snailedlt's comment below, I thought we would work on this after the release.

@ConX Yes, that was just a temp fix to avoid all the errors :) We should add it back ASAP after the upcoming release

Snailedlt · 2023-05-16T19:16:54Z

@ConX
Yeah, let's do this after the release :)

ConX and others added 4 commits March 28, 2023 00:04

Add reference to checkout/v3

69c5e01

Force changes to svg file to test optimize

2c372b1

Force changes to svg file to test optimize

b97cbce

Icons optimized by SVGO

95c7cf7

ConX requested review from lunatic-fox and Snailedlt March 28, 2023 05:51

ConX added 3 commits March 28, 2023 00:54

Force changes to svg file to test optimize on source repo

10c0720

Revert to unedited aarch64-original

aa01faf

Add workflow_dispatch to execute manually for...

6250b45

...existing PRs.

lunatic-fox added 3 commits March 28, 2023 07:39

Test 001 - trigger and reference

2a90f17

Update optimize_icons.yml

c28358b

Revert changes

351fb76

ConX added 3 commits March 30, 2023 00:20

Add debug message

f0fb3ac

Merge branch 'exp-optimize-icons' of https://github.com/ConX/devicon …

a63df66

…into exp-optimize-icons

Switch event trigger to "pull_request_target

a3a8204

Revert aarch64 icons

1aa3466

lunatic-fox approved these changes Mar 30, 2023

View reviewed changes

lunatic-fox requested a review from Panquesito7 March 30, 2023 06:46

Snailedlt reviewed Mar 30, 2023

View reviewed changes

lunatic-fox self-requested a review March 30, 2023 09:04

lunatic-fox mentioned this pull request Apr 10, 2023

Bugfix: Remove Optimize Icons workflow from develop branch #1724

Merged

1 task

Panquesito7 requested changes Apr 28, 2023

View reviewed changes

ConX closed this by deleting the head repository May 17, 2023

ConX mentioned this pull request May 18, 2023

Add optimize_icons workflow triggered on push #1764

Draft

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: Fix optimize_icons workflow error on PRs #1718

Bugfix: Fix optimize_icons workflow error on PRs #1718

ConX commented Mar 28, 2023 •

edited

Loading

lunatic-fox commented Mar 28, 2023

lunatic-fox commented Mar 28, 2023

ConX commented Mar 28, 2023

ConX commented Mar 28, 2023 •

edited

Loading

ConX commented Mar 30, 2023

lunatic-fox left a comment

Snailedlt left a comment

lunatic-fox commented Mar 30, 2023

Snailedlt commented Mar 30, 2023

ConX commented Mar 31, 2023

Snailedlt commented Apr 1, 2023

Panquesito7 left a comment

ConX commented May 3, 2023

Snailedlt commented May 3, 2023

Panquesito7 commented May 3, 2023

lunatic-fox commented May 3, 2023

Panquesito7 commented May 13, 2023 •

edited

Loading

ConX commented May 13, 2023

Snailedlt commented May 16, 2023

Bugfix: Fix optimize_icons workflow error on PRs #1718

Bugfix: Fix optimize_icons workflow error on PRs #1718

Conversation

ConX commented Mar 28, 2023 • edited Loading

Double check these details before you open a PR

Features

Notes

lunatic-fox commented Mar 28, 2023

lunatic-fox commented Mar 28, 2023

ConX commented Mar 28, 2023

ConX commented Mar 28, 2023 • edited Loading

ConX commented Mar 30, 2023

lunatic-fox left a comment

Choose a reason for hiding this comment

Snailedlt left a comment

Choose a reason for hiding this comment

lunatic-fox commented Mar 30, 2023

Snailedlt commented Mar 30, 2023

ConX commented Mar 31, 2023

Snailedlt commented Apr 1, 2023

Panquesito7 left a comment

Choose a reason for hiding this comment

ConX commented May 3, 2023

Snailedlt commented May 3, 2023

Panquesito7 commented May 3, 2023

lunatic-fox commented May 3, 2023

Panquesito7 commented May 13, 2023 • edited Loading

ConX commented May 13, 2023

Snailedlt commented May 16, 2023

ConX commented Mar 28, 2023 •

edited

Loading

ConX commented Mar 28, 2023 •

edited

Loading

Panquesito7 commented May 13, 2023 •

edited

Loading