-
Notifications
You must be signed in to change notification settings - Fork 79
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
feat: new configuration system, config subcommand #736
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #736 +/- ##
==========================================
- Coverage 60.36% 59.32% -1.04%
==========================================
Files 189 203 +14
Lines 6776 7460 +684
==========================================
+ Hits 4090 4426 +336
- Misses 2072 2400 +328
- Partials 614 634 +20 ☔ View full report in Codecov by Sentry. |
f895daf
to
8b2856d
Compare
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.
Overall, I like how it is structured.
I think a few refactoring passes are needed before this can be merged.
I also noticed that the viper library already ships our previous toml library, so might be shipping both. Are we ok with the binary size?
I'll put the toml library change into another PR and then we can compare binary sizes. I don't think it's a huge difference. Edit: See #758 |
dd2eda9
to
69096d4
Compare
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.
Partial review (its a huge PR ;) ), will continue tomorrow :)
This would technically be possible, but has a lot of drawbacks:
I considered the approach you described while implementing the spec, however the current approach offers way more concise code (it only takes a call of the
Feel free to comment those parts in your review. |
I don't think it increases the duplication too much, it is moving some code around. Instead of having "complex" code in the business logic, we have dumb code in the config package (at least we have a dumb API, and hide the complexity).
I am not sure if this is a real problem, we could have dedicated config struct for testing that embed the normal config struct.
I prefer to clutter the config interface instead of the business code. Ensuring a proper separation of concern with simple APIs, will make the project easier to work with.
Will do. |
That said, I don't have a strong opinion. I just wanted to discuss the topic, to see if we can have an even better API. |
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.
This is getting better and better, I like it!
I added the few comment in places I though we could deduplicate/move some code.
In the current implementation you need:
If options were implemented on the config interface, you would need:
This is tedious and would definitely need to be documented somewhere. Also it could be easy to forget some steps, especially for someone in the future who didn't read this PR.
I'm not sure if I understand you correctly here, but that's not how structs work in Go. You'd need an interface to abstract everything away (which would mean even more code duplication)
Fair point, but I don't think
That's completely valid. As I said, I first followed the approach you proposed too when implementing the spec, but it really didn't make that much sense. It might have a prettier outer-facing API, but remember that we have to maintain the actual implementation as well. |
Alright, case close from my side then. |
Co-authored-by: Jonas L. <jooola@users.noreply.github.com>
acd7fb5
to
bf5e964
Compare
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.
Awesome! Looking really good now :)
(We will for sure find some bugs and rough edges after merging, but that happens always).
This PR adds tests to the `context` subcommands using the new configuration system implemented in #736
When a `*pflag.FlagSet` is parsed, it outputs a usage string if the `--help` flag is included. Since Cobra already does this, this behavior needs to be disabled or the usage will be printed twice. --- Since #736 hasn't been release yet, this fix shouldn't appear in the changelog. BEGIN_COMMIT_OVERRIDE feat: new configuration system, config subcommand END_COMMIT_OVERRIDE
🤖 I have created a release *beep* *boop* --- ## [1.44.0](v1.43.1...v1.44.0) (2024-06-20) ### Features * delete multiple resources in parallel ([#761](#761)) ([f2fb321](f2fb321)) * improve toml formatting ([#758](#758)) ([eacb7dd](eacb7dd)) * **load-balancer:** allow specifying health check options in add-service ([#743](#743)) ([2cd08b2](2cd08b2)), closes [#742](#742) * new action waiting progress ([#749](#749)) ([9e30f3f](9e30f3f)) * new configuration system, config subcommand ([#736](#736)) ([d1c6678](d1c6678)) * **server-type:** add deprecated column to list command ([#780](#780)) ([906f864](906f864)) * **server:** add default-ssh-keys option ([#759](#759)) ([9b34d26](9b34d26)) ### Bug Fixes * **firewall:** 'create --rules-file' not working with outbound rules ([#752](#752)) ([2f2be32](2f2be32)), closes [#750](#750) * network list server count format ([#783](#783)) ([f69d261](f69d261)) * track progress if the terminal width allows it ([#768](#768)) ([069fffe](069fffe)), closes [#767](#767) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This PR implements the new configuration system as described in #762.
Closes #762
In preparation for #434