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

Add configure flags command for configuring CLI behavior #436

Merged
merged 11 commits into from
Feb 15, 2024
Merged

Conversation

Piccirello
Copy link
Contributor

This PR adds the ability to configure various CLI behavior via a new configure flags command. The following flags default to true and can be disabled via configure flags disable or via setup with a valid doppler.yaml.

  • analytics: Whether anonymous analytics are enabled.
  • env-warning: Whether to print a warning when CLI behavior is modified via an environment variable.
  • update-check: Whether to check for CLI updates.

Closes ENG-6729, closes #422, closes #407

Comment on lines +69 to +78
if version.PerformVersionCheck && configuration.CanReadEnv {
enable := os.Getenv("DOPPLER_ENABLE_VERSION_CHECK")
if enable == "false" {
logValueFromEnvironmentNotice("DOPPLER_ENABLE_VERSION_CHECK")
version.PerformVersionCheck = false
}
}
if version.PerformVersionCheck {
version.PerformVersionCheck = !utils.GetBoolFlagIfChanged(cmd, "no-check-version", !version.PerformVersionCheck)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relocated this logic from loadFlags() since the config file isn't loaded yet when that function runs.

nmanoogian
nmanoogian previously approved these changes Oct 17, 2023
Copy link
Member

@nmanoogian nmanoogian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loving this impl, glad you went this direction 👍

I did some light testing locally but your E2Es look pretty comprehensive!

pkg/cmd/configure_flags.go Outdated Show resolved Hide resolved
nmanoogian
nmanoogian previously approved these changes Oct 18, 2023
@agilesteel
Copy link

Any updates here? The issues this PR closes are ~8 months old, so it would be really great to merge this one in.

@nmanoogian
Copy link
Member

Any updates here? The issues this PR closes are ~8 months old, so it would be really great to merge this one in.

Thanks for the bump, @agilesteel! I'll be taking this PR over and I should have it fixed up and ready to go shortly.

We were getting the following gosec error when using `slices.Contains`:

```
could not import slices (invalid package name: "")
```

This is because:

- `slices` was added in Go 1.21
- Support for Go 1.21 was added in gosec 2.17
- We are currently using gosec 2.15.0 and via salus 3.2.5 via federacy/scan-action which pulls `coinbase/salus:latest`

Salus 3.2.6 has been released with the gosec version bump but it isn't tagged `latest`. I've filed coinbase/salus#880 to address.

For now, the easiest thing to do was just to not use the `slices` module, which was easy because we already have a util which does the same thing.
Updated initially to fix the gosec issue described in the previous commit but it didn't help. Kept the change anyway.
@nmanoogian nmanoogian merged commit 2522b47 into master Feb 15, 2024
28 checks passed
@nmanoogian nmanoogian deleted the tom-flags branch February 15, 2024 16:25
@agilesteel
Copy link

Bro for real we've been waiting for 8 months and you fixed it in 2 days. Epic! 👏🏼

@nmanoogian
Copy link
Member

Bro for real we've been waiting for 8 months and you fixed it in 2 days. Epic! 👏🏼

Full credit goes to @Piccirello! He parted ways with Doppler before he could get this merged and all that was missing was fixing a small but tricky issue in the tests.

The delay since late October is on me; I apologize for that and we have improved our processes to make sure that PRs aren't allowed to sit for this long in the future.

@Piccirello
Copy link
Contributor Author

@nmanoogian thank you for getting this over the finish line. I see this capability potentially serving future customization needs, beyond just the several people already asking for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants