chore: Introduce 'check' task #3715
Conversation
CONTRIBUTING.md
Outdated
| $ yarn check:MY_CHECK | ||
| ``` | ||
|
|
||
| Where `MY_CHECK` can be any of those: |
There was a problem hiding this comment.
[pp] This might be quite outdated soon. In might be easier to point to the package.json/
| @@ -61,6 +61,7 @@ | |||
| "postversion": "yarn generate --force && yarn doc && ts-node scripts/after-bump.ts", | |||
| "lint": "turbo run lint", | |||
| "lint:fix": "turbo run lint:fix --force", | |||
| "checks": "turbo run check && yarn run check:circular && yarn run check:license && yarn run check:test-service", | |||
There was a problem hiding this comment.
[q] Why are the checks split between prerequisites for the turbo step and the turbo step? I assume that this is because check:dependencies and check:public-api does not need compilation? Does check:license need it? And do the other checks really "depend on" check:dependencies?
I wonder if this is the reason, can we make it more explicit? Maybe with something like pre-compile-checks and post-compile-checks and then combining them in one check?
There was a problem hiding this comment.
Some of the checks use turbo, and some of them don't (that's the status quo).
My understanding from what @florian-richter explained about the reasoning is that turbo is used for steps which have scripts in packages and thus can benefit from being run in parallel, compared to steps which only operate on the whole project where turbo would not benefit us.
I'm not sure if it has anything todo with 'needs compile' or not.
My first impulse was to put all checks into turbo, but then I wanted to keep the change relatively small and I did not see the large benefit, that's why I've ended up with this multi-command for what "checks" does.
There was a problem hiding this comment.
Ok, but then still: Why does the turbo run check step (semantically) depend on the dependency check. The turbo run check should (semantically) execute the dependency check. Technically it will be executed, but the configuration seems unintended to me.
There was a problem hiding this comment.
This might be my missing knowledge about how to configure turbo, but is there another way to express this? Looking at the docs I don't see it, and we don't seem to do anything else in any of our tasks in turbo json..
| }, | ||
| "test": { | ||
| "dependsOn": ["^compile", "^generate"] | ||
| }, | ||
| "check": { | ||
| "dependsOn": ["^compile", "check:dependencies", "check:public-api"] |
There was a problem hiding this comment.
[q] Do the packages themselves have a check script? Would the result be the same if there is a check script that executes check:dependencies and check:public-api and only depends on compile.
I am not sure if one approach is preferable than the other. The way it is in the PR now works, so I'm fine if we do it like this.
There was a problem hiding this comment.
I don't understand your comment. I think both dependencies and public-api are in all packages, but the other checks are not.
I don't have a strong opinion on that. Are there reasons why one would want to actually install without compile? Thinking about it, I'd like to separate this discussion because I do see possible arguments why removing the postinstall might be a bad idea which are not related to the intention of this PR. |
| @@ -61,6 +61,7 @@ | |||
| "postversion": "yarn generate --force && yarn doc && ts-node scripts/after-bump.ts", | |||
| "lint": "turbo run lint", | |||
| "lint:fix": "turbo run lint:fix --force", | |||
| "checks": "turbo run check && yarn run check:circular && yarn run check:license && yarn run check:test-service", | |||
There was a problem hiding this comment.
Just a thought, but as discussed, this would loose turbo's parallelization
| "checks": "turbo run check && yarn run check:circular && yarn run check:license && yarn run check:test-service", | |
| "checks": "yarn check:dependencies && yarn check:public-api && yarn check:circular && yarn check:license && yarn check:test-service", |
Introduce new 'check' task in build, remove dependency-check from compile task.
This change introduces a unified way to run all checks, making it more simple to verify that all of them succeed when developing.
Closes SAP/cloud-sdk-backlog#1076.