-
Notifications
You must be signed in to change notification settings - Fork 653
[rush-lib] Add environment variable to override allowWarningsInSuccessfulBuild setting #2705
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
Conversation
| this._validateCommandLineConfigCommand(command); | ||
|
|
||
| const overrideAllowWarnings: boolean = | ||
| this.rushConfiguration && EnvironmentConfiguration.allowWarningsInSuccessfulBuild; |
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.
Why do we need this.rushConfiguration && here?
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.
Was working with Emmanuel on this one... this is early enough in the command line parser that it's possible rush.json doesn't exist (for example, we might be preparing to run "rush init" in an empty folder).
Because we don't know what command we are about to run, it's possible that rush configuration is undefined at this point, and if rush configuration is undefined then the EnvironmentConfiguration is not initialized.
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.
This makes a assumption that rushConfiguration !== undefined implies that EnvironmentConfiguration is initialized, which seems brittle. (Is that guaranteed? Is that the only way it can be initialized?)
Maybe a simpler solution is to ensure that the EnvironmentConfiguration is initialized before we access it.
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.
Was chatting more about this offline and I agree, and I think what we might do is:
- Change
EnvironmentConfigurationso that it initializes itself if it hasn't been initialized yet. - Continue to initialize (without reset) at key narrow paths like RushConfiguration, where it makes sense to capture and error on the majority of possible configuration errors.
- Possibly some other small tweaks to the error behavior of EnvironmentConfiguration.
These changes (especially #3) probably deserve their own dedicated review, so if it's cool with you we can make those changes in a follow-up PR.
|
What's the practical use case for this feature? Why would you want your CI to be more lax than your developers' local builds? |
|
@iclanton (Emmanuel is on vacation this week, so filling in while he's away.)
The situation we have is that some (not all) projects in the monorepo can't be fully tested without a deployment to a device and/or HTTP server. So we have a deployment strategy that allows for any developer to put up a PR build and it will automatically trigger deploys of any changed projects to branch-specific URLs that they can interact with, share with designers and QA folks, etc., before merging the PR. When you put up your PR, the deploy job wants to run "rush build" and then a custom deploy script; but if the rush build fails, we aren't going to deploy. The net effect is that TS and eslint warnings block the deployment of your build for testing, which is often not desirable.
From the developer's perspective, CI isn't "more lax" in this case -- when they run "rush build" locally, it spits out warnings and lint errors, but it will still finish the full build. (Yes, it produces exit code 1, but that's not relevant generally to the developer.) Once the PR goes up, the standard PR build is definitely going to fail and prevent merging because of the lint warnings, but they still want the deploy job to succeed unless there was a fatal error (like a compilation failed, for example). That's the reason for wanting to control whether warnings or allowed by job, and the easiest way to do that in most CI pipelines is using environment variables. |
|
@elliot-nelson - cool, your scenario makes sense. |
Summary
This PR adds a new environment variable that allows builds to exit with a 0 exit code, even though there are warnings that would normally cause the build to return a non-0 exit code.
Details
The
command-line.jsonconfiguration file enables you to specify theallowWarningsInSuccessfulBuildoption but does not give the liberty to change it in a pipeline or CI build, which could be useful in situations where you want to allow warnings only in certain pipelines. (This setting will override the option for ALL commands, but thats okay -- the pipeline can set the environment variable for each rush command they intend to run if desired)How it was tested
Tested manually for: value 1, value 0, invalid values, etc, and for the default (no environment variable set).
(Per convention did not add a specific unit test for this variable)