search

devicons / devicon

Set of icons representing programming languages, designing & development tools
https://devicon.dev
MIT License
9.19k stars 2.26k forks source link

Bugfix: Fix optimize_icons workflow error on PRs #1718

Closed ConX closed 1 year ago

ConX commented 1 year ago

Double check these details before you open a PR

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.

lunatic-fox commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

  1. Edit the PULL_REQUEST_TEMPLATE for new icons to include a checkbox to ask the contributor to enable `Allow edits by maintainers".
  2. 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 commented 1 year ago

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.

ConX commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Yeah, I think we should go for the preflight workflow option with workflow_run. This is how we fixed the in-develop labeler: https://github.com/devicons/devicon/pull/1410

ConX commented 1 year ago

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 commented 1 year ago

@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 :)

ConX commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Panquesito7 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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