-
Notifications
You must be signed in to change notification settings - Fork 1
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
Phased Builds DIFF (do not merge) #1
Open
elliot-nelson
wants to merge
24
commits into
main
Choose a base branch
from
phased
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
c86fcaa
phased only: edits to command-line.json
elliot-nelson 25aa052
phased only: edits to rush-project.json
elliot-nelson c78b0df
merge main
elliot-nelson c3465e1
Merge branch 'main' into phased
elliot-nelson d24d5df
update names
elliot-nelson e676596
add proposed syntax for skipping phases
elliot-nelson bbfbaa7
fix rush-project.json format
elliot-nelson 50787d0
Some phased command-related tweaks
iclanton 86dc71e
Merge pull request #2 from iclanton/ianc/phased-updates
elliot-nelson eb5f694
Updates to reflect schema changes.
iclanton 94c73de
Merge pull request #3 from iclanton/ianc/update-schema
elliot-nelson 019912b
Move incremental back to the commands.
iclanton 5ce910c
Merge pull request #4 from iclanton/ianc/update-schema
elliot-nelson 73ac083
Add a missint .eslintrc.js
iclanton 035e877
Update the build cache entry name pattern.
iclanton 328c3dd
Update the experiment name.
iclanton b621ad5
Update experiment name.
iclanton c6f3e30
Merge pull request #5 from iclanton/phased-fixes
elliot-nelson 6fcf66e
Remove the explicit "rebuild" command.
iclanton 875b76b
Merge pull request #6 from iclanton/remove-rebuild
elliot-nelson 0bc45e1
Update rush-project.json schema.
iclanton b593536
Merge pull request #7 from iclanton/update-rush-project.json
elliot-nelson fa281fb
Comment out parameter with addPhasesToCommand and skipPhasesForComman…
iclanton d5c9afb
Merge pull request #8 from iclanton/remove-phase-control-options
elliot-nelson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| // This is a workaround for https://github.com/eslint/eslint/issues/3458 | ||
| require('@rushstack/eslint-config/patch/modern-module-resolution'); | ||
|
|
||
| module.exports = { | ||
| extends: [ '@rushstack/eslint-config/profile/web-app' ], | ||
| parserOptions: { tsconfigRootDir: __dirname } | ||
| }; |
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
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
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
29 changes: 28 additions & 1 deletion
29
rigs/acme-classic-rig/profiles/library/config/rush-project.json
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,30 @@ | ||
| { | ||
| "projectOutputFolderNames": ["lib", "lib-commonjs", "dist", "temp"] | ||
| // Structure proposed by @octogonz for phase options | ||
| // | ||
| // Folders like "temp/coverage" are (probably) already folders written to by | ||
| // your unit tests, while folders like "temp/lint" are possible places we could | ||
| // have phases write their log outputs to (since the phase is likely not to have | ||
| // any other tangible output files to cache). | ||
| "operationSettings": [ | ||
| { | ||
| "operationName": "_phase:compile", | ||
| "outputFolderNames": ["lib", "lib-commonjs", "dist", "temp/compile"] | ||
| }, | ||
| { | ||
| "operationName": "_phase:lint", | ||
| "outputFolderNames": ["temp/lint"] | ||
| }, | ||
| { | ||
| "operationName": "_phase:test", | ||
| "outputFolderNames": ["temp/test", "temp/jest-reports", "temp/coverage"] | ||
| }, | ||
| { | ||
| "operationName": "_phase:update-readme", | ||
| "outputFolderNames": ["temp/update-readme"] | ||
| }, | ||
| { | ||
| "operationName": "_phase:push-notes", | ||
| "outputFolderNames": ["temp/push-notes"] | ||
| } | ||
| ] | ||
| } |
29 changes: 28 additions & 1 deletion
29
rigs/acme-classic-rig/profiles/web-app/config/rush-project.json
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,30 @@ | ||
| { | ||
| "projectOutputFolderNames": ["lib", "lib-commonjs", "dist", "temp"] | ||
| // Structure proposed by @octogonz for phase options | ||
| // | ||
| // Folders like "temp/coverage" are (probably) already folders written to by | ||
| // your unit tests, while folders like "temp/lint" are possible places we could | ||
| // have phases write their log outputs to (since the phase is likely not to have | ||
| // any other tangible output files to cache). | ||
| "operationSettings": [ | ||
| { | ||
| "operationName": "_phase:compile", | ||
| "outputFolderNames": ["lib", "lib-commonjs", "dist", "temp/compile"] | ||
| }, | ||
| { | ||
| "operationName": "_phase:lint", | ||
| "outputFolderNames": ["temp/lint"] | ||
| }, | ||
| { | ||
| "operationName": "_phase:test", | ||
| "outputFolderNames": ["temp/test", "temp/jest-reports", "temp/coverage"] | ||
| }, | ||
| { | ||
| "operationName": "_phase:update-readme", | ||
| "outputFolderNames": ["temp/update-readme"] | ||
| }, | ||
| { | ||
| "operationName": "_phase:push-notes", | ||
| "outputFolderNames": ["temp/push-notes"] | ||
| } | ||
| ] | ||
| } |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are these phases mapped to Rush actions? I think we're missing the definition in
"commands".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a realistic example, we should show the intended Rush shell commands for realistic everyday tasks:
--litebuild where we skip unit tests and linting?rush rebuild?Maybe the answer is that initially each of these problems requires a completely new command action name. (
rush build-lite?) But if so we can still enumerate them to get a sense of how painful that feature cut will be in practice.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are great questions.
Since presumably the monorepo had this
--liteneed before phases, I implemented a solution inmain(a custom build script that lets it properly handle--litefrom the rushx command line).That
--liteflag is rough for the phased approach though. As an example, I imagined that the user wantedbuild.jsto pass--liteboth to Heft and to thepush_notes.jscustom script. If I invent a new phased commandbuild-lite, then it can have phases['_phase:compile', '_phase:readme_updater']etc., and you don't need to pass--liteto Heft because you've baked that into the phase. But that's not generically possible if you were doing something with the--liteflag.I guess one possible approach is to implement lite phases,
push-notesandpush-notes-lite, and only include the appropriate one. But they'll never share build cache, and would have to be leaf nodes (no dependents), otherwise how would a dependent phase know which one to rely on.