From 0d55e7723f8529f4793fc824e606209d6c64ae43 Mon Sep 17 00:00:00 2001 From: meowjesty Date: Mon, 25 Sep 2023 12:02:09 -0300 Subject: [PATCH 1/3] Change config::verify to warn on conflict between targetless + steal mode when called from an IDE. --- mirrord/cli/src/extension.rs | 2 +- mirrord/cli/src/main.rs | 4 ++-- mirrord/config/src/lib.rs | 21 ++++++++++++++++++--- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/mirrord/cli/src/extension.rs b/mirrord/cli/src/extension.rs index 29219f651b6..cb953052eee 100644 --- a/mirrord/cli/src/extension.rs +++ b/mirrord/cli/src/extension.rs @@ -61,7 +61,7 @@ pub(crate) async fn extension_exec(args: ExtensionExecArgs, watch: drain::Watch) let mut analytics = AnalyticsReporter::only_error(config.telemetry, watch); - config.verify(&mut context)?; + config.verify::(&mut context)?; for warning in context.get_warnings() { progress.warning(warning); } diff --git a/mirrord/cli/src/main.rs b/mirrord/cli/src/main.rs index 60f7e6a6693..d166e429ac0 100644 --- a/mirrord/cli/src/main.rs +++ b/mirrord/cli/src/main.rs @@ -223,7 +223,7 @@ async fn exec(args: &ExecArgs, watch: drain::Watch) -> Result<()> { let mut analytics = AnalyticsReporter::only_error(config.telemetry, watch); (&config).collect_analytics(analytics.get_mut()); - config.verify(&mut context)?; + config.verify::(&mut context)?; for warning in context.get_warnings() { progress.warning(warning); } @@ -466,7 +466,7 @@ async fn verify_config(VerifyConfigArgs { path }: VerifyConfigArgs) -> Result<() let verified = match LayerFileConfig::from_path(path) .and_then(|config| config.generate_config(&mut config_context)) .and_then(|config| { - config.verify(&mut config_context)?; + config.verify::(&mut config_context)?; Ok(config) }) { Ok(config) => VerifiedConfig::Success { diff --git a/mirrord/config/src/lib.rs b/mirrord/config/src/lib.rs index 496bdbb2599..35856b4dce6 100644 --- a/mirrord/config/src/lib.rs +++ b/mirrord/config/src/lib.rs @@ -301,9 +301,18 @@ impl LayerConfig { } /// Verify that there are no conflicting settings. + /// /// We don't call it from `from_env` since we want to verify it only once (from cli) - /// Returns vec of warnings - pub fn verify(&self, context: &mut ConfigContext) -> Result<(), ConfigError> { + /// + /// Fills `context` with the warnings. + /// + /// `WARN_TARGETLESS` changes the config conflict (targetless + steal mode) to a warning, + /// instead of a hard error, as the IDEs can still recover from this by showing a target pick + /// dialog. + pub fn verify( + &self, + context: &mut ConfigContext, + ) -> Result<(), ConfigError> { if self.pause { if self.agent.ephemeral && !self.agent.privileged { context.add_warning("The target pause feature with ephemeral requires to enable the privileged flag on the agent.".to_string()); @@ -367,9 +376,15 @@ impl LayerConfig { if self.target.namespace.is_some() { Err(ConfigError::TargetNamespaceWithoutTarget)?; } + if self.feature.network.incoming.is_steal() { - Err(ConfigError::Conflict("Steal mode is not compatible with a targetless agent, please either disable this option or specify a target.".into()))?; + if WARN_TARGETLESS { + context.add_warning("Steal mode is not compatible with a targetless agent, please either disable this option or specify a target.".into()); + } else { + Err(ConfigError::Conflict("Steal mode is not compatible with a targetless agent, please either disable this option or specify a target.".into()))?; + } } + if self.agent.ephemeral { Err(ConfigError::Conflict( "Using an ephemeral container for the agent is not compatible with a targetless agent, please either disable this option or specify a target.".into(), From 42241ab7e0cdc9c880f60e6bd86e3277d1897b30 Mon Sep 17 00:00:00 2001 From: meowjesty Date: Mon, 25 Sep 2023 13:47:00 -0300 Subject: [PATCH 2/3] error -> warning --- mirrord/cli/src/extension.rs | 2 +- mirrord/cli/src/main.rs | 4 ++-- mirrord/config/src/lib.rs | 17 ++--------------- 3 files changed, 5 insertions(+), 18 deletions(-) diff --git a/mirrord/cli/src/extension.rs b/mirrord/cli/src/extension.rs index cb953052eee..29219f651b6 100644 --- a/mirrord/cli/src/extension.rs +++ b/mirrord/cli/src/extension.rs @@ -61,7 +61,7 @@ pub(crate) async fn extension_exec(args: ExtensionExecArgs, watch: drain::Watch) let mut analytics = AnalyticsReporter::only_error(config.telemetry, watch); - config.verify::(&mut context)?; + config.verify(&mut context)?; for warning in context.get_warnings() { progress.warning(warning); } diff --git a/mirrord/cli/src/main.rs b/mirrord/cli/src/main.rs index d166e429ac0..60f7e6a6693 100644 --- a/mirrord/cli/src/main.rs +++ b/mirrord/cli/src/main.rs @@ -223,7 +223,7 @@ async fn exec(args: &ExecArgs, watch: drain::Watch) -> Result<()> { let mut analytics = AnalyticsReporter::only_error(config.telemetry, watch); (&config).collect_analytics(analytics.get_mut()); - config.verify::(&mut context)?; + config.verify(&mut context)?; for warning in context.get_warnings() { progress.warning(warning); } @@ -466,7 +466,7 @@ async fn verify_config(VerifyConfigArgs { path }: VerifyConfigArgs) -> Result<() let verified = match LayerFileConfig::from_path(path) .and_then(|config| config.generate_config(&mut config_context)) .and_then(|config| { - config.verify::(&mut config_context)?; + config.verify(&mut config_context)?; Ok(config) }) { Ok(config) => VerifiedConfig::Success { diff --git a/mirrord/config/src/lib.rs b/mirrord/config/src/lib.rs index 35856b4dce6..18cc4ed460f 100644 --- a/mirrord/config/src/lib.rs +++ b/mirrord/config/src/lib.rs @@ -305,14 +305,7 @@ impl LayerConfig { /// We don't call it from `from_env` since we want to verify it only once (from cli) /// /// Fills `context` with the warnings. - /// - /// `WARN_TARGETLESS` changes the config conflict (targetless + steal mode) to a warning, - /// instead of a hard error, as the IDEs can still recover from this by showing a target pick - /// dialog. - pub fn verify( - &self, - context: &mut ConfigContext, - ) -> Result<(), ConfigError> { + pub fn verify(&self, context: &mut ConfigContext) -> Result<(), ConfigError> { if self.pause { if self.agent.ephemeral && !self.agent.privileged { context.add_warning("The target pause feature with ephemeral requires to enable the privileged flag on the agent.".to_string()); @@ -376,15 +369,9 @@ impl LayerConfig { if self.target.namespace.is_some() { Err(ConfigError::TargetNamespaceWithoutTarget)?; } - if self.feature.network.incoming.is_steal() { - if WARN_TARGETLESS { - context.add_warning("Steal mode is not compatible with a targetless agent, please either disable this option or specify a target.".into()); - } else { - Err(ConfigError::Conflict("Steal mode is not compatible with a targetless agent, please either disable this option or specify a target.".into()))?; - } + context.add_warning("Steal mode is not compatible with a targetless agent, please either disable this option or specify a target.".into()); } - if self.agent.ephemeral { Err(ConfigError::Conflict( "Using an ephemeral container for the agent is not compatible with a targetless agent, please either disable this option or specify a target.".into(), From 99f2b94b718a31ddddc3e76258ded597374f94ad Mon Sep 17 00:00:00 2001 From: meowjesty Date: Mon, 25 Sep 2023 14:09:29 -0300 Subject: [PATCH 3/3] changelog --- changelog.d/verify-config.changed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/verify-config.changed.md diff --git a/changelog.d/verify-config.changed.md b/changelog.d/verify-config.changed.md new file mode 100644 index 00000000000..26b9d9eb700 --- /dev/null +++ b/changelog.d/verify-config.changed.md @@ -0,0 +1 @@ +Change targetless + steal mode to warning instead of error. \ No newline at end of file