Skip to content

[rush] Configure operation "dependsOnEnvVars" setting#3769

Merged
iclanton merged 5 commits into
microsoft:mainfrom
elliot-nelson:enelson/env-vars
Nov 21, 2022
Merged

[rush] Configure operation "dependsOnEnvVars" setting#3769
iclanton merged 5 commits into
microsoft:mainfrom
elliot-nelson:enelson/env-vars

Conversation

@elliot-nelson
Copy link
Copy Markdown
Collaborator

@elliot-nelson elliot-nelson commented Nov 19, 2022

Summary

Allow maintainers to configure dependsOnEnvVars property, per-operation, in the rush-project.json configuration file.

Any variables configured this way have their keys and values included in the build cache hash.

Fixes #3730.

Details

NAMING

In the linked issue one suggestion is cachingEnvVariables, but I think this doesn't make it clear enough that it is an "input" to the build operation. But, for (future-redesign-of-rush-project-json-reasons), I think it's useful to reserve the word "input" only for files that live inside the project folder and are tracked by git.

dependsOnEnvVars is intended to align with "depends on other phases", "depends on other projects", and (perhaps in future) "depends on additional files outside this folder" -- anything the operation "depends" on becomes part of its hash.

CHANGES

  • Added a relatively generic additionalContext string bag to the build cache, in hopes of making this very simple to port to the new structure in PR [rush] Rework ProjectChangeAnalyzer, operation hashes #3696.

  • Updated rush-project schema and loading logic, using a merge operation when inheritance (the same logic as the existing output folder names).

SUGGESTED TIMING

It seems like this change is relatively orthogonal to PR #3696: it'd be useful to have the feature now, and shouldn't significantly impact that PR. But if we want to fold this into those changes that's OK too.

How it was tested

  • Local monorepo: adding vars to list immediately invalidates cache
  • Local monorepo: changing env var value invalidates cache
  • Local monorepo: can read existing entries when env var value matches a previously cached value

Comment thread common/changes/@microsoft/rush/enelson-env-vars_2022-11-19-18-14.json Outdated
Comment thread libraries/rush-lib/src/logic/buildCache/ProjectBuildCache.ts Outdated
@iclanton
Copy link
Copy Markdown
Member

Something else we could consider: We could add an option to only allow specifically enumerated env vars to be available during operation runtime.

This change would be totally unrelated to this change.

@elliot-nelson
Copy link
Copy Markdown
Collaborator Author

Something else we could consider: We could add an option to only allow specifically enumerated env vars to be available during operation runtime.

This change would be totally unrelated to this change.

👍 I was looking at bazel and turbo while thinking about the naming of this option and I noticed bazel only passes down whitelisted environment variables.

We wouldn't have to make it unrelated to this change -- for example, we could make the dependsOnEnvVars setting control the list for both, and a boolean option "strictEnv" could apply that list to the ones we pass down.

(Although we'd probably need some other settings, like a global whitelist somewhere else, as well.)

Comment thread libraries/rush-lib/src/logic/buildCache/ProjectBuildCache.ts Outdated
Comment thread libraries/rush-lib/src/logic/operations/ShellOperationRunner.ts
@iclanton
Copy link
Copy Markdown
Member

👍 I was looking at bazel and turbo while thinking about the naming of this option and I noticed bazel only passes down whitelisted environment variables.

Yeah BuildXL does that too.

We wouldn't have to make it unrelated to this change -- for example, we could make the dependsOnEnvVars setting control the list for both, and a boolean option "strictEnv" could apply that list to the ones we pass down.

I think it should be a separate change.

elliot-nelson and others added 2 commits November 21, 2022 07:36
Sort included context keys

Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
@iclanton iclanton enabled auto-merge November 21, 2022 19:10
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.

[rush] Specify environment variables for build cache

3 participants