Skip to content

Commit

Permalink
fix(affected): don't respect empty scm env vars (#9053)
Browse files Browse the repository at this point in the history
### Description

Users can set empty string env vars which will end up getting chosen
over our defaults. Empty env vars can accidentally get set when using
[`docker
compose`](https://docs.docker.com/compose/environment-variables/set-environment-variables/#additional-information)
if a pass through env is declared in the YAML, but the shell does not
have the var defined.

This means that `Some("")` will no longer be a valid value in for
`from_ref`. This is already the case for `TargetSelector`s from
`--filter`s:
[source](https://github.com/vercel/turborepo/blob/main/crates/turborepo-lib/src/run/scope/target_selector.rs#L145)

This also allows us to remove the awkward comparison where we check if
the scm base and skip calculating changes if it is `Some("")`, but do
the calculation if it is `None`.
### Testing Instructions

Added unit test to verify that empty scm env vars will not be respected.
  • Loading branch information
chris-olszewski authored Aug 23, 2024
1 parent 933d3a2 commit ca29f0f
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 21 deletions.
8 changes: 6 additions & 2 deletions crates/turborepo-lib/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,11 +299,11 @@ impl ConfigurationOptions {
}

pub fn scm_base(&self) -> Option<&str> {
self.scm_base.as_deref()
non_empty_str(self.scm_base.as_deref())
}

pub fn scm_head(&self) -> &str {
self.scm_head.as_deref().unwrap_or("HEAD")
non_empty_str(self.scm_head.as_deref()).unwrap_or("HEAD")
}

pub fn allow_no_package_manager(&self) -> bool {
Expand Down Expand Up @@ -961,6 +961,8 @@ mod test {
env.insert("turbo_daemon".into(), "".into());
env.insert("turbo_env_mode".into(), "".into());
env.insert("turbo_preflight".into(), "".into());
env.insert("turbo_scm_head".into(), "".into());
env.insert("turbo_scm_base".into(), "".into());

let config = get_env_var_config(&env).unwrap();
assert_eq!(config.api_url(), DEFAULT_API_URL);
Expand All @@ -972,6 +974,8 @@ mod test {
assert_eq!(config.daemon, None);
assert_eq!(config.env_mode, None);
assert!(!config.preflight());
assert_eq!(config.scm_base(), None);
assert_eq!(config.scm_head(), "HEAD");
}

#[test]
Expand Down
35 changes: 16 additions & 19 deletions crates/turborepo-lib/src/run/scope/change_detector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,26 +93,23 @@ impl<'a> GitChangeDetector for ScopeChangeDetector<'a> {
include_uncommitted: bool,
allow_unknown_objects: bool,
) -> Result<HashSet<PackageName>, ResolutionError> {
let mut changed_files = HashSet::new();
if !from_ref.map_or(false, |s| s.is_empty()) {
changed_files = match self.scm.changed_files(
self.turbo_root,
from_ref,
to_ref,
include_uncommitted,
allow_unknown_objects,
)? {
ChangedFiles::All => {
debug!("all packages changed");
return Ok(self
.pkg_graph
.packages()
.map(|(name, _)| name.to_owned())
.collect());
}
ChangedFiles::Some(changed_files) => changed_files,
let changed_files = match self.scm.changed_files(
self.turbo_root,
from_ref,
to_ref,
include_uncommitted,
allow_unknown_objects,
)? {
ChangedFiles::All => {
debug!("all packages changed");
return Ok(self
.pkg_graph
.packages()
.map(|(name, _)| name.to_owned())
.collect());
}
}
ChangedFiles::Some(changed_files) => changed_files,
};

let lockfile_contents = self.get_lockfile_contents(from_ref, &changed_files);

Expand Down

0 comments on commit ca29f0f

Please sign in to comment.