-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix noUpdateNotifier not suppressing updates #10941
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
base: main
Are you sure you want to change the base?
Fix noUpdateNotifier not suppressing updates #10941
Conversation
Move update notification check to shim to respect NO_UPDATE_NOTIFIER env var. Co-authored-by: anthony.shew <anthony.shew@vercel.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Cursor Agent can help with this pull request. Just |
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.
Additional Comments:
crates/turborepo-lib/src/config/turbo_json.rs (lines 48-82):
The no_update_notifier field from turbo.json is never transferred to ConfigurationOptions, causing the noUpdateNotifier setting to be completely ignored. This means users cannot disable update notifications via turbo.json configuration.
View Details
📝 Patch Details
diff --git a/crates/turborepo-lib/src/config/turbo_json.rs b/crates/turborepo-lib/src/config/turbo_json.rs
index 1041c18b8..89df31aef 100644
--- a/crates/turborepo-lib/src/config/turbo_json.rs
+++ b/crates/turborepo-lib/src/config/turbo_json.rs
@@ -78,6 +78,9 @@ impl<'a> TurboJsonReader<'a> {
opts.env_mode = turbo_json.env_mode.map(|mode| *mode.as_inner());
opts.cache_dir = cache_dir;
opts.concurrency = turbo_json.concurrency.map(|c| c.as_inner().clone());
+ opts.no_update_notifier = turbo_json
+ .no_update_notifier
+ .map(|n| *n.as_inner());
opts.future_flags = turbo_json.future_flags.map(|f| *f.as_inner());
Ok(opts)
}
@@ -227,4 +230,27 @@ mod test {
let config = TurboJsonReader::turbo_json_to_config_options(turbo_json).unwrap();
assert_eq!(config.allow_no_package_manager(), expected);
}
+
+ #[test_case(None, false)]
+ #[test_case(Some(false), false)]
+ #[test_case(Some(true), true)]
+ fn test_no_update_notifier(value: Option<bool>, expected: bool) {
+ let turbo_json = RawRootTurboJson::parse(
+ &serde_json::to_string_pretty(
+ &(if let Some(value) = value {
+ json!({
+ "noUpdateNotifier": value
+ })
+ } else {
+ json!({})
+ }),
+ )
+ .unwrap(),
+ "turbo.json",
+ )
+ .unwrap()
+ .into();
+ let config = TurboJsonReader::turbo_json_to_config_options(turbo_json).unwrap();
+ assert_eq!(config.no_update_notifier(), expected);
+ }
}
Analysis
Missing field transfer causes noUpdateNotifier configuration to be ignored
What fails: The turbo_json_to_config_options() function in crates/turborepo-lib/src/config/turbo_json.rs (line 48) does not transfer the no_update_notifier field from RawTurboJson to ConfigurationOptions, causing the noUpdateNotifier setting in turbo.json to be completely ignored.
How to reproduce:
- Create a
turbo.jsonfile with{"noUpdateNotifier": true} - The field is properly deserialized into
RawTurboJson.no_update_notifier(defined at line 136 ofturbo_json/raw.rs) - During conversion in
turbo_json_to_config_options(), the field is never assigned toopts.no_update_notifier - The
ConfigurationOptions.no_update_notifier()method returnsfalse(default) instead oftrue - The check
!config.no_update_notifier()inshim/mod.rs:319fails to suppress update notifications
Result: Users cannot disable update notifications via turbo.json configuration. The setting is silently ignored.
Expected: The no_update_notifier field should be transferred during conversion, following the same pattern as other boolean fields like daemon and allow_no_package_manager (lines 77-78 of turbo_json.rs).
Evidence: The field exists in all necessary structs:
RawTurboJson(line 136 ofturbo_json/raw.rs):pub no_update_notifier: Option<Spanned<bool>>ConfigurationOptions(line 323 ofconfig/mod.rs):pub(crate) no_update_notifier: Option<bool>- Getter method (line 479 of
config/mod.rs):pub fn no_update_notifier(&self) -> bool
But the conversion in lines 73-83 of turbo_json.rs transfers 7 other fields (token, ui, allow_no_package_manager, daemon, env_mode, cache_dir, concurrency, future_flags) while omitting no_update_notifier.
Description
Fixes an issue where
noUpdateNotifier: trueinturbo.jsondid not suppress update notifications. The update notification logic was refactored to centralize all suppression checks (config and environment variables) in the caller, removing a redundant check that ignored theturbo.jsonsetting.Testing Instructions
"noUpdateNotifier": trueto yourturbo.json.turboCLI version is older than the latest available.turbocommand (e.g.,turbo run build).Linear Issue: TURBO-4878