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

ci: allow multi submission #2295

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

Conversation

ReenigneArcher
Copy link

@ReenigneArcher ReenigneArcher commented Nov 5, 2024

Double check these details before you open a PR

  • PR does not match another non-stale PR currently opened

Features

This PR allows multi submission edits in a single PR, as discussed in #2270.

This PR closes NONE

Notes

I have tested this against the same branch that creates #2270 in my local fork. This is the resulting comment: ReenigneArcher#1 (comment). All of the comments are valid issues, so I believe this is working as expected.

Note, that somewhere in the code empty strings are getting added to the error list. I don't believe this is anything from my changes, but I have just removed the empties using the following line.

err_msg = list(filter(None, err_msg)) # remove empty strings from err_msg

Some trailing whitespace was also automatically removed from lines in modified files.

@Snailedlt FYI

@Snailedlt Snailedlt requested review from a team, ConX, weh, Snailedlt, canaleal and lunatic-fox and removed request for a team November 5, 2024 08:00
@Snailedlt
Copy link
Collaborator

Great!
I'll see if I can take a look at the PR this weekend

Copy link
Collaborator

@Snailedlt Snailedlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very good!
However I think we might get issues with the release script, since that also builds icons based on the PR title.
I might be wrong here, but iirc that's a different script, which will also need to be refactored.

To test out if this works with the release script(s), you can go through the release guide on your fork, and see if you get any errors, and if every step executes as expected.

If you find any ways to make the release process simpler, please feel free to adjust it as you see fit :)

@ReenigneArcher
Copy link
Author

However I think we might get issues with the release script, since that also builds icons based on the PR title.

Thanks for the review. I'll dig into that in the next few days and report back.

@canaleal canaleal added the feature:icon Use this label for pull requests when a new icon is ready to be added to the collection label Nov 17, 2024
@ReenigneArcher

This comment was marked as resolved.

@Snailedlt Snailedlt added devops Use this label for devops related enhancements and removed feature:icon Use this label for pull requests when a new icon is ready to be added to the collection labels Dec 5, 2024
@ReenigneArcher ReenigneArcher force-pushed the ci/allow-multi-submission branch 4 times, most recently from fc615c1 to f813c4a Compare December 5, 2024 21:39
@ReenigneArcher

This comment was marked as resolved.

@Snailedlt
Copy link
Collaborator

@ReenigneArcher hmmm, that's very odd. No idea sadly... but maybe it helps to recreate the repo instead of forking it? You could also try re-forking it

@ReenigneArcher

This comment was marked as resolved.

@ReenigneArcher

This comment was marked as resolved.

@ReenigneArcher

This comment was marked as resolved.

@Snailedlt
Copy link
Collaborator

@ReenigneArcher Now that #2310 has been merged, this PR should be possible to test, right?

@ReenigneArcher
Copy link
Author

@Snailedlt yes, I believe so. I just need to re-gather my thoughts on this as it has been a a while since I looked at all this.

@ReenigneArcher ReenigneArcher force-pushed the ci/allow-multi-submission branch from f813c4a to 9436f25 Compare February 12, 2025 00:38
@ReenigneArcher

This comment was marked as resolved.

@Snailedlt
Copy link
Collaborator

  1. The bot peek script stopped working a long time ago. I believe because of an PAI change, or some bandwidth limit in the API. When it worked it would post a few screenshots of how the icon font looks when uploaded to icomoon. It would be awesome if we got it working again, since it's a hazzle to upload and check the font manually... But it's not required to get this PR merged. So feel free to try and fix it, or delete it. Whichever you want :)
  2. I've got no idea sadly. Logging the arguments might be a good place to start though. I think you're right about it exiting before that line, maybe even before that method is called. Maybe you can try changing to named arguments, since that makes it quite a bit easier to read.

@ReenigneArcher

This comment was marked as resolved.

@ReenigneArcher

This comment was marked as resolved.

@ReenigneArcher ReenigneArcher force-pushed the ci/allow-multi-submission branch 2 times, most recently from 68a90b5 to 23c24d8 Compare February 13, 2025 02:55
@ReenigneArcher ReenigneArcher marked this pull request as draft February 13, 2025 04:04
@Snailedlt
Copy link
Collaborator

I figured out why there was no output. It seems the output is redirected to a log file which is then uploaded as an artifact.

Right, iirc it's uploaded as an artifact, and then later on the artifact is retrieved. It's a weird workaround for a very odd security issue with allowing workflows to run on forked repos.
Here's an explanation of why we need to upload the artifact in a preflight script, and then download it again to use it: #1099

This is also related: #1715

KeyError: "There is no item named 'fonts/devicon.eot' in the archive"

Hmm, seems there is something wrong with the font creation in icomoon. devicon.eot is a file that should be downloaded from icomoon when creating the font.
Maybe you could try creating the font manually.

I also recommend reading about how we build the font in our wiki to get a better grasp of how it all works. It's been a while since I read it myself, so maybe I should too 😅
Anyways, here are some links for you:

This wiki page is also very useful for an overview on most of our automated workflows, and it should also be updated when we update this one: https://github.com/devicons/devicon/wiki/Our-automated-tasks-and-bots

@Snailedlt
Copy link
Collaborator

Hmm, it might also be that icomoon has renamed the .eot file to something else, or that the GitHub cache is stuck or something.
I would suggest trying these two things first:

  1. Delete the cache for the workflow, and rerun it to see if that fixes anything
  2. Do the font creation process manually, to better understand how it works, and to see if the filenames and locations are as expected.

@ReenigneArcher

This comment was marked as resolved.

@Snailedlt
Copy link
Collaborator

Sounds like a good plan. Let me know if you get stuck :)

@ReenigneArcher

This comment was marked as resolved.

@Snailedlt
Copy link
Collaborator

Snailedlt commented Feb 15, 2025

Hmm, that's interesting 🤔

So I'm guessing the issue is somewhere in the build_icons.yml script then. Either that or icomoon changes their API.

Have you checked if the .eot file is created when doing it on the master branch?

If it is, then there must be something we changed after the last release that broke it, and if not it's likely something external, because I don't remember having any issues with a missing .eot file when making the last release

@ReenigneArcher

This comment was marked as resolved.

@ReenigneArcher
Copy link
Author

I found the issue with the missing .eot file. The fix is in #2361

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.

3 participants