CLI: Fix sb init in Yarn workspace environment
#10985
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gaetanmaisse
added
bug
cli
compatibility with other tools
yarn / npm
gaetanmaisse
commented
May 29, 2020
| version: 'latest', | ||
| generator: [ | ||
| 'cd {{name}}-v{{version}}', | ||
| 'echo "{ \\"name\\": \\"workspace-root\\", \\"private\\": true, \\"workspaces\\": [] }" > package.json', |
There was a problem hiding this comment.
Init a Yarn workspace with the most simple way (i.e. without any tricks or new dependency)
gaetanmaisse
force-pushed
the
fix-10915-cli-init-in-yarn-workspace
branch
3 times, most recently
from
a388860
to
ef9e891
Compare
Add `--ignore-workspace-root-check` to avoid Yarn error when adding dependencies to the workspace root.
gaetanmaisse
force-pushed
the
fix-10915-cli-init-in-yarn-workspace
branch
from
9a9a796
to
d0ef50a
Compare
Removed this option because it can hide some issues with dependency installation process of @storybook/cli. For instance, missing Yarn flags needed to have the CLI working in Yarn workspaces. Also, as `sb init` is now running `yarn/npm install` the resolution step has to be made before init Storybook.
gaetanmaisse
force-pushed
the
fix-10915-cli-init-in-yarn-workspace
branch
from
d0ef50a
to
82e418f
Compare
With the previous algorithm, some nodes have no jobs assigned to them due to rounding consideration. For example with 10 nodes and 13 jobs to assign: 13 jobs / 10 nodes -> 1.3 jobs per node -> rounded to 2 -> only the 1st 6 nodes run 2 jobs, the 7th 1 job and the other 0 job. Using a modulo ensures that each node will at least run 1 job.
gaetanmaisse
force-pushed
the
fix-10915-cli-init-in-yarn-workspace
branch
from
a39832b
to
2345957
Compare
gaetanmaisse
requested review from
igor-dv,
ndelangen,
shilman and
stijnkoopal
as code owners
tooppaaa
approved these changes
May 29, 2020
| const nodeIndex = +process.env.CIRCLE_NODE_INDEX || 0; | ||
| const numberOfNodes = +process.env.CIRCLE_NODE_TOTAL || 1; | ||
|
|
||
| const list = narrowedConfigs.filter((_, index) => { |
There was a problem hiding this comment.
I wanted to do that for a while now ^^
This logic is shared in other runners, chromatic and examples if I'm right
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: #10915
What I did
--ignore-workspace-root-checkflag when CLI is runningyarn addcommand.skip-installoption fromsb initcommand used in E2E because it can hide some issues with the dependency installation process of @storybook/cli. For instance, missing Yarn flags needed to have the CLI working in Yarn workspaces.--
Also, improve jobs assignment on CI nodes:
With the current algorithm, some nodes have no jobs assigned to them due to rounding consideration. For instance, with 10 nodes and 13 jobs to assign:
Using a modulo ensures that each node will at least run 1 job and thus reduce the E2E run duration (in theory 🤞).
How to test
example-v2job