Yak shaving CI

[ayyaruq]

@nonowazu complained builds were slow on his PR so you can have them about 15 seconds faster.

Actual fixes: the PR type filters were useless (although can keep them if you want), caching on package-lock is pointless if npm install is ignoring it (because install always uses package.json and will update if it can, which defeats the purpose of a lock), and enabled the cache that didn't seem to be used on python tests.

Pointless fixes: clarity and overly verbose names that give no additional output. Prob could rename some of the python jobs tho.

Branch name featuring dzo because those are yaks in FFXIV blame Koji not me.

This is largely useless but yolo. I left the other workflows alone because I saw Windows and ran screaming into the early evening.

NOTE: PR will fail until package-lock.json is removed from gitignore and committed like good paprikas. CI should never be updating your deps. I made the PR to add caching tho so here we are.

[panicstevenson]

the PR type filters were useless (although can keep them if you want)

I had added these to prevent actions running from the other pull_request types (assigned, closed, etc.), but it looks like opened, synchronize, and reopened are the defaults, so this should be fine.

overly verbose names that give no additional output

These were named for people that might not be familiar with what GitHub actions (or git in general) might know what's going on. I don't necessarily care personally if they stay or go. I would probably agree and argue that they provide so little benefit that they're probably not worth the line of code, but I'll defer to someone else here.

Also, I believe we ran into issues with checkout@v2, leading to us downgrading it.

[ayyaruq]

Also, I believe we ran into issues with checkout@v2, leading to us downgrading it.

I believe this was fixed in August upstream (the linked issue doesn't mention it but the original issue it's a dupe of is closed. No big deal to revert my change there tho, functionally as a user there's no major diff between the 2 versions unless you're actually passing extra params to it.

Re PR event types, stuff like closed etc don't trigger by default as you mentioned, so the original list was just duplicating what the default already did and making the actions hard to read. For now I'll revert the checkout action version in the internet of "it literally doesn't matter", package-lock.json stuff is pending discussion I had with quis on Discord (basically removing it from gitignore and committing his local version of it since its authoritative).

[ayyaruq]

So idk how I feel about this but basically, latest commit avoids checking if cache was used or not. The cache action seems broken as of like 2 days ago in that it's not properly restoring. Both pip and npm should just skip stuff after a brief lookup if they see they're already installed in the cached directory, so basically check takes longer while cache is broken but at the same time it should be using it when it loads properly.

[quisquous]

Overall, I don't see any issues with the changes as-is. Looks good. It sure looks like it saves 10-15 seconds on dependencies over previous jobs, comparing to a bunch of other actions in the past. This is still a draft pull request. Is there anything left you wanted to change?

Thanks for reverting back to checkout v1. If we bump that, I'd rather do it in its own pr because of the past issues.

Is there an upstream issue for the changes you made in 67067d5? Do you have numbers for how long it takes npm to check vs the action check, or is that just your feeling that it's slower? Mostly just curious!

@panicstevenson did you have any other opinions here?

[panicstevenson]

@panicstevenson did you have any other opinions here?

Nope, LGTM.

[ayyaruq]

I can't find a ref for the issue and I don't get why the pip cache isn't working as expected, likely a path issue (although it should be fine since black and pylint seem to be installing to the right place). At least for node I know it's due to native modules, although I wonder about caching node_modules directly since it's a single build target only (which is the usual reason not to). Any thoughts on what to change or just merge since it still is an improvement already?

@panicstevenson?

[panicstevenson]

I don't think I followed the last comment, sorry. I see the cache working in your latest GitHub actions runs:

Cache restored from key: Linux-pip-380e07d0b888c224f2673712eaa9c1cb03b78fd9a9222e2697c4121977ae5e66

and

Run pip install -r requirements.txt
Collecting black==19.10b0
  Using cached black-19.10b0-py36-none-any.whl (97 kB)
Collecting pylint==2.5.3
  Using cached pylint-2.5.3-py3-none-any.whl (324 kB)
Collecting PyYAML
  Using cached PyYAML-5.3.1.tar.gz (269 kB)
Collecting requests
  Using cached requests-2.25.0-py2.py3-none-any.whl (61 kB)
Collecting attrs>=18.1.0
  Using cached attrs-20.3.0-py2.py3-none-any.whl (49 kB)
Collecting toml>=0.9.4
  Using cached toml-0.10.2-py2.py3-none-any.whl (16 kB)

Or at least that feels like it's working. Is the thought here that it's not skipping the install entirely? You reverted the change for cache hits 67067d5, which feels like the right move overall based on the documentation and the fact that I can't even build if I add them back in (also assuming this is why these changes were backed out), although it basically makes the cache a lot less attractive, as mentioned in actions/cache#423.

Apologies in advance if I completely missed the question, the intent, or both.

To answer the last part, I think it is still an improvement nonetheless!

[ayyaruq]

I missed the cache loading bit, oops. I suppose it's fine for python then (although previously failed, unsure why). With node, can prob leave as is for now. I'd consider adding node_modules to the cache given it's a single OS job, it'll run slightly faster for lack of compiling native modules, while still preserving the download cache if needed, but maybe later. I'll convert to a proper PR too.

[ayyaruq]

Actually real quick - is there any reason for black and pylint to run as separate jobs in parallel? While parallelism is generally good, in this case it's not really any faster and if either fails you want to fail the build anyway.

[panicstevenson]

Actually real quick - is there any reason for black and pylint to run as separate jobs in parallel? While parallelism is generally good, in this case it's not really any faster and if either fails you want to fail the build anyway.

The only reason would be so that they give separate outputs / clarity of what fails, but I think that people touching Python in general are somewhat familiar with what they're doing, since the Python bits are generally developer-centric. I'll let @quisquous have final say on that, if they have any strong feelings.

[quisquous]

Actually real quick - is there any reason for black and pylint to run as separate jobs in parallel? While parallelism is generally good, in this case it's not really any faster and if either fails you want to fail the build anyway.

The only reason would be so that they give separate outputs / clarity of what fails, but I think that people touching Python in general are somewhat familiar with what they're doing, since the Python bits are generally developer-centric. I'll let @quisquous have final say on that, if they have any strong feelings.

I think it doesn't matter too much, but being separate might make it easier to debug issues in the future if black and pylint happen to disagree on some corner case.

[ayyaruq]

Maiko's feedback addressed, ready for merge now.

[quisquous]

Thank you for this!!