feat: new configuration system, config subcommand #736

phm07 · 2024-04-19T11:11:14Z

This PR implements the new configuration system as described in #762.

Closes #762
In preparation for #434

codecov · 2024-04-19T11:13:02Z

Codecov Report

Attention: Patch coverage is 46.60326% with 393 lines in your changes missing coverage. Please review.

Project coverage is 59.32%. Comparing base (f2fb321) to head (ceeb3ab).
Report is 6 commits behind head on main.

❗ Current head ceeb3ab differs from pull request most recent head 458746b

Please upload reports for the commit 458746b to get more accurate results.

Files	Patch %	Lines
internal/state/config/config.go	0.00%	126 Missing ⚠️
internal/state/config/options.go	11.23%	79 Missing ⚠️
internal/cmd/config/config.go	0.00%	36 Missing ⚠️
internal/state/config/context.go	0.00%	29 Missing ⚠️
internal/cmd/config/set.go	65.82%	21 Missing and 6 partials ⚠️
internal/cmd/config/remove.go	64.06%	15 Missing and 8 partials ⚠️
internal/cmd/config/add.go	68.25%	15 Missing and 5 partials ⚠️
internal/state/config/preferences.go	74.64%	15 Missing and 3 partials ⚠️
internal/state/state.go	0.00%	11 Missing ⚠️
internal/cmd/config/unset.go	81.81%	6 Missing and 2 partials ⚠️
... and 5 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #736      +/-   ##
==========================================
- Coverage   60.14%   59.32%   -0.83%     
==========================================
  Files         191      203      +12     
  Lines        6808     7484     +676     
==========================================
+ Hits         4095     4440     +345     
- Misses       2099     2402     +303     
- Partials      614      642      +28

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

internal/cmd/cmpl/suggestions.go

internal/cmd/firewall/describe.go

jooola

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?

internal/cmd/config/config.go

internal/cmd/config/helptext/generate.go

internal/cmd/config/list.go

internal/state/config/config.go

internal/state/config/options.go

phm07 · 2024-05-10T14:23:18Z

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

…tion subcommand

apricote

Partial review (its a huge PR ;) ), will continue tomorrow :)

internal/cmd/firewall/add_rule.go

internal/cmd/firewall/delete_rule.go

internal/cmd/firewall/describe.go

internal/cmd/firewall/describe_test.go

internal/cmd/util/util.go

internal/state/state.go

…ontext active error message

jooola

Took another look.

Do you think it would be possible to add the config.OptionQuiet options inside the config struct? This way we would only have the config struct in the state and get the value from it?

So instead of config.OptionQuiet.Get(s.Config()) we would simply have s.Config().Quiet() ?

Other than that, there is a bunch of code that can be deduplicated, extracted in smaller functions, so the higher level logic is easier to understand.

internal/state/config/config.go

internal/cmd/cmpl/suggestions.go

internal/state/config/config.go

phm07 · 2024-06-05T10:11:30Z

Do you think it would be possible to add the config.OptionQuiet options inside the config struct? This way we would only have the config struct in the state and get the value from it?

So instead of config.OptionQuiet.Get(s.Config()) we would simply have s.Config().Quiet() ?

This would technically be possible, but has a lot of drawbacks:

There would be a lot of code duplication. There would need to be a method implemented on the config struct for every single option. Also there would need to be a map that stores all option. There is no way to create such a map automatically when getter methods are used to access options. Right now newOpt is used to handle that.
Testing would be a lot harder since options would be hard coded. Currently, the option set is extended for some tests. This is for example used to test nested options which don't exist yet.
It would unnecessarily clutter the Config interface.

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 newOpt function to create a new option) and the only trade-off is that the code is slightly less pretty.

Other than that, there is a bunch of code that can be deduplicated, extracted in smaller functions, so the higher level logic is easier to understand.

Feel free to comment those parts in your review.

jooola · 2024-06-05T10:27:38Z

There would be a lot of code duplication. There would need to be a method implemented on the config struct for every single option. Also there would need to be a map that stores all option. There is no way to create such a map automatically when getter methods are used to access options. Right now newOpt is used to handle that.

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).

Testing would be a lot harder since options would be hard coded. Currently, the option set is extended for some tests. This is for example used to test nested options which don't exist yet.

I am not sure if this is a real problem, we could have dedicated config struct for testing that embed the normal config struct.

It would unnecessarily clutter the Config interface.

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.

Feel free to comment those parts in your review.

Will do.

jooola · 2024-06-05T10:30:35Z

There would be a lot of code duplication. There would need to be a method implemented on the config struct for every single option. Also there would need to be a map that stores all option. There is no way to create such a map automatically when getter methods are used to access options. Right now newOpt is used to handle that.

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.

jooola

This is getting better and better, I like it!

I added the few comment in places I though we could deduplicate/move some code.

internal/cmd/config/add.go

internal/cmd/config/get.go

internal/cmd/config/add.go

internal/cmd/config/remove.go

internal/cmd/config/set.go

internal/cmd/config/list.go

internal/cmd/config/set.go

phm07 · 2024-06-05T10:47:45Z

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).

In the current implementation you need:

One variable declaration using the newOpt method and you're done. Everything else is done for you.

If options were implemented on the config interface, you would need:

Some sort of variable declaration for the option, since it would need to be re-used multiple times (see below)
A function declaration for the getter on the Config interface
An implementation of the getter method for each struct that implements the Config interface (variable would be needed here)
Some piece of code that adds the option to a global map which contains all options (needed so that options can be iterated over and can be retrieved by name; variable would be needed here too)

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 am not sure if this is a real problem, we could have dedicated config struct for testing that embed the normal config struct.

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)

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.

Fair point, but I don't think config.Option<...>.Get(s.Config()) is clutter. config.Option<...> allows you to autocomplete all valid options and config.OptionX.<...> lets you autocomplete all functions than can be performed on options, like overriding them, getting values or getting the type. Making it so that it accepts a config.Config makes it easily mockable.

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.

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.

jooola · 2024-06-05T12:49:11Z

README.md

-can configure a per-directory context by setting `HCLOUD_CONTEXT=my-context` via `.envrc`.
+## Configuring hcloud
+
+The hcloud CLI tool allows configuration using following methods:


Suggested change

The hcloud CLI tool allows configuration using following methods:

The hcloud CLI tool can be configured using following methods:

How about "can be configured"?

Yes work for me.

README.md

internal/cmd/config/util.go

jooola · 2024-06-05T13:09:26Z

config.OptionQuiet.Get(s.Config()) => s.Config().Quiet()

Alright, case close from my side then.

Co-authored-by: Jonas L. <jooola@users.noreply.github.com>

phm07 force-pushed the configuration branch 3 times, most recently from f895daf to 8b2856d Compare May 2, 2024 12:33

apricote reviewed May 3, 2024

View reviewed changes

internal/cmd/cmpl/suggestions.go Show resolved Hide resolved

internal/cmd/firewall/describe.go Outdated Show resolved Hide resolved

jooola reviewed May 8, 2024

View reviewed changes

phm07 force-pushed the configuration branch from 7b9881f to 172ae8c Compare May 10, 2024 14:15

phm07 mentioned this pull request May 10, 2024

test(context): add unit tests #755

Draft

phm07 force-pushed the configuration branch 3 times, most recently from dd2eda9 to 69096d4 Compare May 23, 2024 12:29

phm07 changed the title ~~feat: add global/per-context configuration preferences, add config command~~ feat: new configuration system, config subcommand May 23, 2024

phm07 self-assigned this May 23, 2024

phm07 added the feature label May 23, 2024

phm07 marked this pull request as ready for review May 23, 2024 12:31

phm07 requested a review from a team as a code owner May 23, 2024 12:31

phm07 force-pushed the configuration branch from 69096d4 to 1e7cf7f Compare May 23, 2024 12:37

phm07 and others added 10 commits May 23, 2024 14:39

feat: add global/per-context configuration preferences, add configura…

d2869d7

…tion subcommand

wip

599cbcb

add documentation

180acf5

generate help tables

ec59f6e

Better user facing messages

3b7ce72

Better help texts

88a2ecf

refactor, add tests

9329686

generate

27f6040

tests for preferences

640c3bc

expose Schema

ad7311f

phm07 force-pushed the configuration branch from 1e7cf7f to 7870f13 Compare May 23, 2024 12:39

apricote reviewed May 23, 2024

View reviewed changes

phm07 added 7 commits May 29, 2024 15:03

error if context is not found, fix active context retrieval, adjust c…

ce4b9a6

…ontext active error message

refactor path splitting

8eacd10

add tests for preference validation

763fa5f

only suggest list options for add/remove

d8b02c9

refactor: replace util.IsNil(ctx) with global where possible

a111085

refactor

78b5302

add Config.UseContext(*string), remove Config.Reset(), refactor

ceeb3ab

phm07 force-pushed the configuration branch from d54c018 to ceeb3ab Compare May 29, 2024 13:09

apricote requested a review from jooola May 29, 2024 13:15

jooola reviewed May 31, 2024

View reviewed changes

internal/state/config/config.go Outdated Show resolved Hide resolved

internal/state/config/config.go Outdated Show resolved Hide resolved

internal/cmd/cmpl/suggestions.go Show resolved Hide resolved

internal/state/config/config.go Outdated Show resolved Hide resolved

phm07 added 2 commits June 5, 2024 10:54

config.NewConfig() -> config.New()

0bbc710

refactor config.ReadConfig

0f5e752

phm07 force-pushed the configuration branch from d9d995a to 0f5e752 Compare June 5, 2024 09:49

remove unneeded flag

f369c34

jooola reviewed Jun 5, 2024

View reviewed changes

phm07 added 3 commits June 5, 2024 12:57

add completion for config get

a61a856

refactor, remove duplicate code

ce0e5d3

add OptionFlagSlice, refactor, remove duplicated code

7279a4f

phm07 force-pushed the configuration branch from fc4db0a to 7279a4f Compare June 5, 2024 12:11

move parsing logic to options.go

b81121a

jooola reviewed Jun 5, 2024

View reviewed changes

phm07 and others added 2 commits June 5, 2024 15:37

Update README.md

3f63626

Co-authored-by: Jonas L. <jooola@users.noreply.github.com>

refactor bool parsing

5e5ac80

phm07 force-pushed the configuration branch from b93ab93 to 5e5ac80 Compare June 5, 2024 13:53

add type documentation for options

458746b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new configuration system, config subcommand #736

feat: new configuration system, config subcommand #736

phm07 commented Apr 19, 2024 •

edited

codecov bot commented Apr 19, 2024 •

edited

jooola left a comment

phm07 commented May 10, 2024 •

edited

apricote left a comment

jooola left a comment •

edited

phm07 commented Jun 5, 2024 •

edited

jooola commented Jun 5, 2024 •

edited

jooola commented Jun 5, 2024

jooola left a comment

phm07 commented Jun 5, 2024 •

edited

jooola Jun 5, 2024 •

edited

phm07 Jun 5, 2024

jooola Jun 5, 2024

jooola commented Jun 5, 2024

	The hcloud CLI tool allows configuration using following methods:
	The hcloud CLI tool can be configured using following methods:

feat: new configuration system, config subcommand #736

Are you sure you want to change the base?

feat: new configuration system, config subcommand #736

Conversation

phm07 commented Apr 19, 2024 • edited

codecov bot commented Apr 19, 2024 • edited

Codecov Report

jooola left a comment

Choose a reason for hiding this comment

phm07 commented May 10, 2024 • edited

apricote left a comment

Choose a reason for hiding this comment

jooola left a comment • edited

Choose a reason for hiding this comment

phm07 commented Jun 5, 2024 • edited

jooola commented Jun 5, 2024 • edited

jooola commented Jun 5, 2024

jooola left a comment

Choose a reason for hiding this comment

phm07 commented Jun 5, 2024 • edited

jooola Jun 5, 2024 • edited

Choose a reason for hiding this comment

phm07 Jun 5, 2024

Choose a reason for hiding this comment

jooola Jun 5, 2024

Choose a reason for hiding this comment

jooola commented Jun 5, 2024

phm07 commented Apr 19, 2024 •

edited

codecov bot commented Apr 19, 2024 •

edited

phm07 commented May 10, 2024 •

edited

jooola left a comment •

edited

phm07 commented Jun 5, 2024 •

edited

jooola commented Jun 5, 2024 •

edited

phm07 commented Jun 5, 2024 •

edited

jooola Jun 5, 2024 •

edited