Verify input schemas if present#39
Conversation
Sviatozar Petrenko (foxt451)
requested a review
from Lukáš Křivka (metalwarrior665)
| # let apify cli initialize its config to avoid race conditions with xargs | ||
| apify --version >/dev/null 2>&1 | ||
| if [ -d ./.actor ]; then | ||
| apify vis |
There was a problem hiding this comment.
vis is outdated, we should use validate-schema. It should now validate all schemas
| run: npm run lint | ||
|
|
||
| - name: Install Apify CLI | ||
| run: npm install -g apify-cli |
There was a problem hiding this comment.
I'm a bit worried this will bloat the action runtime a lot. Maybe we could let it work with the current cache but I don't have a good idea how. The current cache is based on hash(package-lock.json) so in this case it will just load the local node modules and will not update the cache after this install since there is no change in the key (I think). JuanGalilea any ideas?
There was a problem hiding this comment.
apify-cli into devDependencies? since that allows us to use it in scripts without knip getting mad at us.
We use the CLI a lot so i dont see a big issue in just adding it to the projects.
This is my knee-jerk solution but i haven't thought much about it. Seems reasonable enough at a glance.
There was a problem hiding this comment.
Not a big fan of that, we would need to add it to all projects and it would bloat all regular installs forever. That's not worth for this check (we do it locally in pre-commit anyway) by me.
But let's first test how long this install takes. If it is like 5 sec, I'm fine with it.
There was a problem hiding this comment.
There was a problem hiding this comment.
just realized these are the shared actions 😢.
Still like the devDependencies idea but its waaaaaayyy harder to implement damn
There was a problem hiding this comment.
I'm assuming CLI is much bigger than things like knip. If not, then Juan's solution would be ok since we already have all these other tools in devDeps
There was a problem hiding this comment.
maybe its worth it to think about custom images?
Interesting. Probably not worth for just npm install apify-cli but maybe we can find more things. I assume there will be some downsides, maybe slower starts.
Lukas Krivka
we could get some more mileage like with tools like knip and some other use everywhere but just for scripts tools.
Definitively out of scope for this PR
There was a problem hiding this comment.
Yeah, it takes 26s, the most out of all steps. But I think the Juan's idea is reasonable, especially since we use apify-cli in a pre-commit, so it's an actual dev dep
There was a problem hiding this comment.
Can you test how much bigger node_modules are with it vs without it (there will be some overlap in sub dependencies)? and what is the install time difference?
There was a problem hiding this comment.
The node_modules size difference is noticeable, 257 - 174 = 83 (in actor for testing actions). It adds 253 packages. In two different action runs the install without apify-cli was a couple of seconds slower, would I need to batch test? Also, what do you think of (I see your reply)npx --yes apify approach below vs skipping the step?
| fi | ||
| if [ -d ./actors ]; then | ||
| # `apify vis` fail code is changed from 1 to 255 so xargs aborts on first error. | ||
| cd ./actors && ls | xargs -P 8 -I {} sh -c 'cd {} && apify vis || exit 255' |
There was a problem hiding this comment.
wouldn't error logs get all mangled with -P 8?
I get the speedup here but people read the failing check logs and if you have errors on >1 actor this would just tangle all the logs afaik.
There was a problem hiding this comment.
We use it like this for a long time and it fail very rarely. If it does you can dig deeper. Definitely not worth to do it 1 by 1 by maybe the parallelization options in GitHub runner is limited and this will be slow anyway...
| apify vis | ||
| fi | ||
| if [ -d ./actors ]; then | ||
| # `apify vis` fail code is changed from 1 to 255 so xargs aborts on first error. |
There was a problem hiding this comment.
i like the solution but i would prefer for all to run and then get hit by ALL the errors, not fix one just to get hit by another.
|
love that you are doing CI work and taking some ownership of it ❤️ |
| run: | | ||
| if [ ! -d "node_modules/apify-cli" ]; then | ||
| echo "::warning::apify-cli is not installed as an npm dependency. Skipping schema validation." | ||
| exit 0 |
There was a problem hiding this comment.
you can run the initialization command as:
npx --yes apify --versionthat way it gets installed on the first command (if missing) and you dont have to exit 0.
It should still show the warning tho since teams should add it to devDependencies. Best of both worlds no?
There was a problem hiding this comment.
yeah, but it would inflate the runtime of repos using this action who haven't added this to their deps by >20 seconds, is it okay? cc Lukáš Křivka (@metalwarrior665)
There was a problem hiding this comment.
also, for knip and prettier we also just skip the step if not installed, so skipping is in line with the present approach. should we stick to it here too?
There was a problem hiding this comment.
tru, but knip is nice to have.
Having the schemas right is important, so its not apples to apples.
Also with the +20s CI increase, each team can easily solve it by just adding it to devDeps so its not like they are just stuck with the bad perf.
Whichever you go for is OK tho, its more opinion based than right/wrong way.
PS: But if you decide to have it do nothing, you should skip the step instead of doing exit 0.
That requires to do the checking in a previous step, emit output and use that in an if clause for this one.
PS2: additionally, if you skip there will be some repos where they don't realize they have the check inactive, since the whole action was green so it must be OK. Whereas in the other version you always get the check so green actually is OK. (even if it took 20s longer)
There was a problem hiding this comment.
Actually giving this anothrr thought, I would drop this completely. Unlike the other checks, this one is hard validated by platform on build. And since we ru this unit test and platform builds in parallel, it just adds to the runtime.without much benefit. So I think keeping it in pre commit only is better
There was a problem hiding this comment.
giving a second thought I agree. I'll close if Juan doesn't have some reasons to keep it
3 participants
Closes #27
Runs on
unitTestjob. Installs apify cli and runsapify vis(mainly taken from existing husky on actor repos), but conditionally checks presence of ./actors/ or .actor/ foldersTested locally with
act