search

saucelabs / saucectl

A command line interface for the Sauce Labs platform.
Apache License 2.0
42 stars 16 forks source link

fix!: Update npm.strictSSL behavior #935

Closed tianfeng92 closed 3 months ago

tianfeng92 commented 3 months ago

Description

Update the strictSSL field to a boolean pointer to distinguish between three states: not set, true, or false.

When it's not set, the npm strictSSL default value of true will be used.

strictSSL not set: https://app.saucelabs.com/tests/22db9a3effdc4d9cbd005839403df483 strictSSL is true: https://app.saucelabs.com/tests/5a57b6dc937c447ea22852c63d5d97c7 strictSSL is false: https://app.saucelabs.com/tests/1ac681507c9a4a8da2f14b1deb6169d9

This update should be merged after chef release.

sauce-runner-utils updates: https://github.com/saucelabs/sauce-runner-utils/pull/76

Besides, mark npm.registry field as deprecated.

tianfeng92 commented 3 months ago

Let me try to explain this and clarify the situation.

I found it quite complicated to manage a field that can be set via both a config file and a command-line flag. For instance, if I don't set the flag, I can easily detect this using Changed(). However, when a config file is involved, it becomes difficult to determine the exact source of the value. For example, if StrictSSL is true, I can't tell whether that value came from the config file or not. As a result, applying the "not set" state isn't feasible. Nonetheless, since the default value is true, this situation is generally acceptable.

For the remaining scenarios, everything works as expected.

Given that StrictSSL is a boolean pointer and the flag is of type bool, here are the scenarios:

  1. Config-driven test only: This works perfectly as expected with no ambiguity(not set/true/false).
  2. CLI-driven test with an empty config file((-c "")): Since there's only one source, I can check whether the flag has changed and if the config file is empty. If so, I can distinguish that it is "not set" and assign a nil pointer to StrictSSL.
  3. CLI-driven test with a config file (-c .sauce/config.yml): In this case, there are two sources. The flag has higher priority than the config file.
    • Neither config file nor flag set: StrictSSL defaults to true, using the flag's default value. While this isn't the "not set" state, the final result is still correct.
    • Only explicitly set in the config file (no --npm.strictSSL flag): StrictSSL follows the value in the config file and ignores the flag's default value.
    • Only explicitly set via flag: StrictSSL follows the flag setting. If the flag is not used, StrictSSL is assigned the default value of true.
    • Set via both config file and flag explicitly: The flag value overrides the config file value.
alexplischke commented 3 months ago

I found it quite complicated to manage a field that can be set via both a config file and a command-line flag. For instance, if I don't set the flag, I can easily detect this using Changed(). However, when a config file is involved, it becomes difficult to determine the exact source of the value. For example, if StrictSSL is true, I can't tell whether that value came from the config file or not. As a result, applying the "not set" state isn't feasible. Nonetheless, since the default value is true, this situation is generally acceptable.

@tianfeng92 Let's say we have a pointer boolean.

The strictSSL config field can have 3 values then: true|false|nil The strictSSL flag can have 3 values as well : true|false|nil (nil determined via Changed()) The flag always takes precedence over the config if it's true|false.

In prose, the logic would be:

if flag has Changed {
  apply flag to config
} else {
  use config as-is
}
tianfeng92 commented 3 months ago

I found it quite complicated to manage a field that can be set via both a config file and a command-line flag. For instance, if I don't set the flag, I can easily detect this using Changed(). However, when a config file is involved, it becomes difficult to determine the exact source of the value. For example, if StrictSSL is true, I can't tell whether that value came from the config file or not. As a result, applying the "not set" state isn't feasible. Nonetheless, since the default value is true, this situation is generally acceptable.

@tianfeng92 Let's say we have a pointer boolean.

The strictSSL config field can have 3 values then: true|false|nil The strictSSL flag can have 3 values as well : true|false|nil (nil determined via Changed()) The flag always takes precedence over the config if it's true|false.

In prose, the logic would be:

if flag has Changed {
  apply flag to config
} else {
  use config as-is
}

For if flag has Changed, apply flag to config, this has been done implicitly. Even without https://github.com/saucelabs/saucectl/pull/935/commits/6a8bf3568cbe8762c8d42c5a10af98906a179d4a

For the case of else use config as-is, the parsed StrictSSL value is a combination of both the flag and the config file. This means I've lost the original value from the config file. In the scenario where neither the flag is passed nor strictSSL is set in the config, the parsed StrictSSL is true, not nil. Even though I know the flag hasn't been changed, I can't determine whether true was explicitly set or if it was simply left empty in the config file.

I think in this case, although the nil state isn't accurate, it still functions correctly.