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.35135% with 397 lines in your changes are missing coverage. Please review.

Project coverage is 59.05%. Comparing base (f2fb321) to head (1838ef6).

Files	Patch %	Lines
internal/state/config/config.go	0.00%	122 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/state/config/preferences.go	69.01%	18 Missing and 4 partials ⚠️
internal/cmd/config/add.go	68.25%	15 Missing and 5 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.05%   -1.10%     
==========================================
  Files         191      204      +13     
  Lines        6808     7519     +711     
==========================================
+ Hits         4095     4440     +345     
- Misses       2099     2436     +337     
- Partials      614      643      +29

☔ 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?

jooola · 2024-05-08T12:45:20Z

internal/cmd/config/config.go

+		Long: `This command allows you to manage options for the Hetzner Cloud CLI. Options can be set inside the
+configuration file, through environment variables or with flags. 
+
+The hierarchy for configuration sources is as follows (from highest to lowest priority):
+1. Flags
+2. Environment variables
+3. Configuration file (context)
+4. Configuration file (global)
+5. Default values
+
+Most options are 'preferences' - these options can be set globally and can additionally be overridden
+for each context. Below is a list of all non-preference options:
+
+` + nonPreferenceOptions +
+			`
+Since the above options are not preferences, they cannot be modified with 'hcloud config set' or 
+'hcloud config unset'. However, you are able to retrieve them using 'hcloud config get' and 'hcloud config list'.
+Following options are preferences and can be used with these commands:
+
+` + preferenceOptions +
+			`
+Options will be persisted in the configuration file. To find out where your configuration file is located
+on disk, run 'hcloud config get config'.
+`,


Why not have everything in external files?

I don't see how that is a benefit. We would have to join the tables in somehow anyway.

internal/cmd/config/helptext/generate.go

internal/cmd/config/list.go

jooola · 2024-05-08T13:06:47Z

internal/state/config/config.go

-	if err := os.MkdirAll(filepath.Dir(cfg.path), 0777); err != nil {
-		return err


Happy not to see this 0777 in the new code, but shouldn't we keep the config dir creation behavior?

Yes, we should. Seems like I forgot to copy it over somewhere. What perms do we want to give that folder? 600 seems a bit restrictive since other tools might also want to access ~/.config/hcloud.

Added in 6eada44

jooola · 2024-05-08T13:12:07Z

internal/state/config/options.go

+	"github.com/hetznercloud/hcloud-go/v2/hcloud"
+)
+
+type OptionFlag int


Should we rename this to OptionFeature? I have and will be confused by this naming more than once.

e.g. !o.HasFlag(OptionFlagEnv) => !o.HasFeature(OptionFeatureEnv)

OptionFlag is a bit flag, that's why it has that name. It's very common to call these bit flags. I would find calling it Feature way more confusing.

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

apricote · 2024-05-23T13:21:48Z

internal/cmd/util/util.go

+// SliceDiff returns the difference between the two passed slices. The returned slice contains all elements that are present in a but not in b.
+// Note that it does not preserve order.
+func SliceDiff[S ~[]E, E cmp.Ordered](a, b []E) []E {
+	m := make(map[E]struct{})
+	for _, x := range a {
+		m[x] = struct{}{}
+	}
+	for _, x := range b {
+		delete(m, x)
+	}
+	var diff S
+	for x := range m {
+		diff = append(diff, x)
+	}
+	slices.Sort(diff)
+	return diff
+}
+
+func AnyToAnySlice(a any) []any {
+	val := reflect.ValueOf(a)
+	if val.Kind() != reflect.Slice {
+		return nil
+	}
+	s := make([]any, val.Len())
+	for i := 0; i < val.Len(); i++ {
+		s[i] = val.Index(i).Interface()
+	}
+	return s
+}
+
+func AnyToStringSlice(a any) []string {
+	var s []string
+	for _, v := range AnyToAnySlice(a) {
+		s = append(s, fmt.Sprint(v))
+	}
+	return s
+}
+
+func ToStringSlice(a []any) []string {
+	var s []string
+	for _, v := range a {
+		s = append(s, fmt.Sprint(v))
+	}
+	return s
+}
+
+func ToAnySlice[T any](a []T) []any {
+	var s []any
+	for _, v := range a {
+		s = append(s, any(v))
+	}
+	return s
+}


Could you add some unit tests for these util functions?

Done in 8e590b1

apricote · 2024-05-23T13:28:16Z

internal/state/state.go

 			opts = append(opts, hcloud.WithDebugWriter(os.Stderr))
 		} else {
-			writer, _ := os.Create(c.debugFilePath)
+			writer, _ := os.Create(filePath)


Unrelated to this PR: I recently noticed that we always truncate the file (os.Create). Pretty annoying if you use hcloud in a script and want all the logs in some file.

I opened #763 so that we can tackle that after this PR

apricote · 2024-05-23T13:30:26Z

internal/state/state.go

+	var opts []hcloud.ClientOption
+
+	token := config.OptionToken.Get(c.config)
+	opts = append(opts, hcloud.WithToken(token))


For slightly simpler code, you could init opts with the options that are always set (token, application)

Suggested change

var opts []hcloud.ClientOption

token := config.OptionToken.Get(c.config)

opts = append(opts, hcloud.WithToken(token))

opts := []hcloud.ClientOption{

hcloud.WithToken(config.OptionToken.Get(c.config)),

hcloud.WithApplication("hcloud-cli", version.Version),

}

(Also need to remove opts = append(opts, hcloud.WithApplication("hcloud-cli", version.Version)))

Makes sense, idk why it was done like that. Done in 1838ef6

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 12 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

make context use schema

e7da132

handle non-existing config file

66abbae

phm07 added 2 commits May 23, 2024 14:39

fix tests

58e3a78

update documentation

7870f13

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 3 commits May 23, 2024 16:26

fix search & replace mistake

c9789c9

add tests for util methods

8e590b1

clean up state creation code

1838ef6

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

jooola May 8, 2024

phm07 May 10, 2024

jooola May 8, 2024

phm07 May 10, 2024

phm07 May 21, 2024

jooola May 8, 2024

phm07 May 10, 2024

phm07 commented May 10, 2024 •

edited

apricote left a comment

apricote May 23, 2024

phm07 May 23, 2024 •

edited

apricote May 23, 2024

phm07 May 23, 2024

apricote May 23, 2024

phm07 May 23, 2024

		if err := os.MkdirAll(filepath.Dir(cfg.path), 0777); err != nil {
		return err

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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

Choose a reason for hiding this comment

phm07 May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phm07 commented Apr 19, 2024 •

edited

codecov bot commented Apr 19, 2024 •

edited

phm07 commented May 10, 2024 •

edited

phm07 May 23, 2024 •

edited