Skip to content
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

[rush] Add an option to not pass phase-control flag parameters on to project commands. #3124

Conversation

iclanton
Copy link
Member

@iclanton iclanton commented Jan 2, 2022

Summary

Currently the design for flag parameters that manipulate which phases are executed conflicts with the design for caching. Using a flag to omit a phase from execution suggests that the phase may not be necessary to get a developer who has just pulled changes into their local clone ready to start work (a _phase:test phase excluded by a --lite flag for example). However, if the flag is also passed on to the _phase:build phase, it ends up being part of the _phase:build's cache entry's hash, meaning that _phase:build won't be able to be restored from cache unless the cache was populated by a rush build --lite run.

I expect that a repo with phased commands and a remote cache will commonly run rush build in CI and then expect developers to run rush build --lite to get going in the morning on their local machines. rush build --lite should pull the output of the _phase:build phase from the cache that was populated by the CI run. This doesn't work if _phase:build's cache entry IDs include the --lite flag.

Details

To address this issue, I propose adding a "doNotIncludeInProjectCommandLine" property to "flag" parameters in common/config/rush/command-line.json that prevents the parameter from being passed on to projects' individual script executions. This property is only allowed for flags that control which phases are executed.

How it was tested

TBD.

@iclanton iclanton changed the title Add an option to not pass phase-control flag parameters on to project commands. [rush] Add an option to not pass phase-control flag parameters on to project commands. Jan 3, 2022
@elliot-nelson
Copy link
Collaborator

elliot-nelson commented Jan 3, 2022

@iclanton Thanks for this PR, I'll respond here (to the PR and your question in the ticket).

I'm still working through the use case here, but in general, no flags are passed to a phase unless that phase is explicitly listed in the associatedPhases property (or at least, that's how I was envisioning it).

So imagine in command-line.json:

{
    "parameterKind": "flag",
    "longName": "--prod",

    "associatedCommands": ["build"],

    "associatedPhases": ["_phase:compile", "_phase:deploy"],
    "addPhasesToCommand": ["_phase:deploy", "_phase:tweet"],
    "skipPhasesForCommand": ["_phase:test"]
}

The flag --prod is going to:

(1) Add phases deploy and tweet to whatever command you run (build in this case).
(2) Skip phase test in whatever command you run (build in this case).
(3) Add the flag --prod to the command line for the compile and deploy phases.

The flag --prod is not added to the tweet phase, for example, because it is not in the associatedPhases list. (Despite the fact we are running --build, the associatedCommands property is really only controlling the help CLI that decides whether rush build --prod is a valid command, not whether to add --prod to each command we run.)

Does that make sense?

EDIT: I realize I didn't actually tie this comment to the change in the PR. In my head, if you add the associatedPhases property and pass the flag down to the underlying command, then the assumption should be that it somehow alters the output of the command and that it must also be included in the cache key. I think that's a valid thing to assume. It's possible that there are some flags that must be passed down to the underlying phase command, but don't change the outputs (perhaps it's slower because it's performing more detailed tests or something). In that case I can see the usefulness of the new property you're proposing, but, it's not tied to this concept of adding/skipping phases then... it's just a property related to whether flags should be part of the cache key or not.

So, I can potentially see the usefulness of the proposed change, but I don't think it should be tied to add/skip.

@iclanton
Copy link
Member Author

iclanton commented Jan 4, 2022

Ah I misinterpreted the design. Real fix is here: #3126

@iclanton iclanton closed this Jan 4, 2022
@iclanton iclanton deleted the dont-pass-phase-flag-params-to-scripts branch January 4, 2022 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants