Skip to content

Conversation

@EmmanuelOluyomi
Copy link
Contributor

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.json configuration file enables you to specify the allowWarningsInSuccessfulBuild option 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)

@ghost
Copy link

ghost commented May 21, 2021

CLA assistant check
All CLA requirements met.

this._validateCommandLineConfigCommand(command);

const overrideAllowWarnings: boolean =
this.rushConfiguration && EnvironmentConfiguration.allowWarningsInSuccessfulBuild;
Copy link
Collaborator

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?

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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:

  1. Change EnvironmentConfiguration so that it initializes itself if it hasn't been initialized yet.
  2. 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.
  3. 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.

@iclanton
Copy link
Member

What's the practical use case for this feature? Why would you want your CI to be more lax than your developers' local builds?

@elliot-nelson
Copy link
Collaborator

@iclanton (Emmanuel is on vacation this week, so filling in while he's away.)

What's the practical use case for this feature?

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.

Why would you want your CI to be more lax than your developers' local builds?

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.

@iclanton
Copy link
Member

@elliot-nelson - cool, your scenario makes sense.

@octogonz octogonz merged commit 02fbdc2 into microsoft:master Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants