Skip to content
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

fix!: Update npm.strictSSL behavior #935

Merged
merged 14 commits into from
Aug 15, 2024
Merged

fix!: Update npm.strictSSL behavior #935

merged 14 commits into from
Aug 15, 2024

Conversation

tianfeng92
Copy link
Contributor

@tianfeng92 tianfeng92 commented Aug 12, 2024

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: saucelabs/sauce-runner-utils#76

Besides, mark npm.registry field as deprecated.

@tianfeng92 tianfeng92 added the breaking-change Pull requests that introduce a breaking change label Aug 12, 2024
@tianfeng92 tianfeng92 changed the title fix: Unify npm.strictSSL behavior fix!: Unify npm.strictSSL behavior Aug 12, 2024
@tianfeng92 tianfeng92 marked this pull request as ready for review August 12, 2024 22:17
@tianfeng92 tianfeng92 requested a review from a team as a code owner August 12, 2024 22:17
tianfeng92 and others added 2 commits August 13, 2024 21:06
Co-authored-by: Alex Plischke <alex.plischke@saucelabs.com>
@tianfeng92 tianfeng92 marked this pull request as draft August 14, 2024 15:55
internal/http/proxy.go Outdated Show resolved Hide resolved
internal/cmd/docker/push.go Outdated Show resolved Hide resolved
tianfeng92 and others added 2 commits August 14, 2024 09:32
Co-authored-by: Alex Plischke <alex.plischke@saucelabs.com>
Co-authored-by: Alex Plischke <alex.plischke@saucelabs.com>
@tianfeng92 tianfeng92 marked this pull request as ready for review August 14, 2024 17:29
@tianfeng92 tianfeng92 changed the title fix!: Unify npm.strictSSL behavior fix!: Standardize npm.strictSSL behavior Aug 14, 2024
@tianfeng92 tianfeng92 changed the title fix!: Standardize npm.strictSSL behavior fix!: Update npm.strictSSL behavior Aug 14, 2024
@tianfeng92
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

tianfeng92 commented Aug 15, 2024

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 6a8bf35

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.

Co-authored-by: Alex Plischke <alex.plischke@saucelabs.com>
@tianfeng92 tianfeng92 merged commit 0644bea into main Aug 15, 2024
16 checks passed
@tianfeng92 tianfeng92 deleted the DEVX-3002 branch August 15, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Pull requests that introduce a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants