fix: zero-values cli flags do not override config file

[GregoryAlbouy]

Description

Due to the behaviour of config.Merge that relies on zero values to decide whether an option should be overridden, it is currently not possible to override a config file option with a CLI option that is a zero value.
For now, it's not that much of an issue since any zero value is an invalid value. However this is not guaranteed to last: in the future we can implement options accepting 0 as a valid value (such as interval) or bools, for which obviously false should be acceptable.

So this PR provides a new overriding logic via Config.Override based on which values are actually set rather than zero values.

Changes

• Replace config.Merge, config.MergeDefault with Config.Override that allows to replace fields selectively
• Fix override baseConfig ⬅️ cliConfig: set fields are retrieved via flag.CommandLine.Visit
• Fix override baseConfig ⬅️ fileConfig: set fields are retrieved via non-nil pointer types in config/file.marshaledConfig

Notes

[codecov-commenter]

Codecov Report

Merging #28 (ab0c5fd) into main (9656fd0) will increase coverage by 2.95%.
The diff coverage is 89.65%.

@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
+ Coverage   59.71%   62.67%   +2.95%     
==========================================
  Files           9       10       +1     
  Lines         278      292      +14     
==========================================
+ Hits          166      183      +17     
+ Misses        101       98       -3     
  Partials       11       11              

Flag	Coverage Δ
unittests	62.67% <89.65%> (+2.95%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
config/file/parser.go	80.00% <ø> (ø)
config/file/parse.go	71.92% <83.78%> (+5.97%)	⬆️
config/config.go	89.47% <100.00%> (+4.99%)	⬆️
config/field.go	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9656fd0...ab0c5fd. Read the comment docs.

[c1-ra]

If find the use of this flagNames() method more elegant than the switch! 👍

[c1-ra]

I think it makes sense that it is a method of Config and not in main. But I have no strong opinion about this, so the first implementation works for me too 🤷‍♀️

[c1-ra]

I have added some comments, but both versions could work as they are for me!

[moreirathomas]

Solves the problem but the api becomes a bit confused:

• we can call Merge from main but it is not intended to be used this way
• Merge is only used through MergeDefault
• Config.Merge can be called from requester when we said that importing package config should not come with a parser (overriding is kinda concerned with parsing...)

Suggestion:
Ideally config exposes only Config struct and Config.HTTPRequest().
We could then have something like config/parse with parse.FromFile(), parse.FromFlags(), parse.Override(base, override config.Config).

Or this could not work, but anyway I think the current API is not really what we want.
We may put this on hold until we have something better and simply merge the simplest fix to the bug ?

[GregoryAlbouy]

Some clarification about the PR (my bad for not pointing that up earlier):

• It's not ready to merge as is: the same fix has to be done for the config file defaulting: because it still uses Merge (with zero values logics), we currently can't override a DefaultConfig option with a zero-value from the config file

• In other words Merge/MergeDefault will be removed: their only usage is from config/file.parseRawConfig, which has to be replaced with Config.Override logics.

• Config.Merge can be called from requester when we said that importing package config should not come with a parser (overriding is kinda concerned with parsing...)

I disagree with that point: there's factually no parsing involved in Merge or Override, only values that are copied or not from one Config to another under certain conditions. They just return a config.Config out of config.Configs, hence they should remain under config.

We could then have something like config/parse with parse.FromFile(), parse.FromFlags()

That's interesting, but indeed not doable in a way that makes sense: all flags are already parsed before we get to call parse.FromFlags, so it becomes parse.FromAlreadyParsedFlags, which is nothing different from config.New.

[moreirathomas]

I see by the commit message that somehow using pointers to type fixes zero-value override.
As I did not see the bug happening, do you care to explain a bit ? :)

[GregoryAlbouy]

Sure! In short, it's only a necessary step to make parseRawConfig, where lies the fix, possible:

runner/config/file/parse.go

Lines 35 to 86 in 22cb9d0

	// parseRawConfig parses an input raw config as a config.Config and returns it
	// or the first non-nil error occurring in the process.
	func parseRawConfig(raw unmarshaledConfig) (config.Config, error) { //nolint:gocognit // acceptable complexity for a parsing func
	cfg := config.Config{}
	fields := make([]config.Field, 0, 6)
	if method := raw.Request.Method; method != nil {
	cfg.Request.Method = *method
	fields = append(fields, config.FieldMethod)
	}
	if rawURL := raw.Request.URL; rawURL != nil {
	parsedURL, err := parseAndBuildURL(*raw.Request.URL, raw.Request.QueryParams)
	if err != nil {
	return config.Config{}, err
	}
	cfg.Request.URL = parsedURL
	fields = append(fields, config.FieldURL)
	}
	if timeout := raw.Request.Timeout; timeout != nil {
	parsedTimeout, err := parseOptionalDuration(*timeout)
	if err != nil {
	return config.Config{}, err
	}
	cfg.Request.Timeout = parsedTimeout
	fields = append(fields, config.FieldTimeout)
	}
	if requests := raw.RunnerOptions.Requests; requests != nil {
	cfg.RunnerOptions.Requests = *requests
	fields = append(fields, config.FieldRequests)
	}
	if concurrency := raw.RunnerOptions.Concurrency; concurrency != nil {
	cfg.RunnerOptions.Concurrency = *concurrency
	fields = append(fields, config.FieldConcurrency)
	}
	if globalTimeout := raw.RunnerOptions.GlobalTimeout; globalTimeout != nil {
	parsedGlobalTimeout, err := parseOptionalDuration(*globalTimeout)
	if err != nil {
	return config.Config{}, err
	}
	cfg.RunnerOptions.GlobalTimeout = parsedGlobalTimeout
	fields = append(fields, config.FieldGlobalTimeout)
	}
	fmt.Println(fields)
	return config.Default().Override(cfg, fields...), nil
	}

Just like for CLI flags with main.configFlags, we need to know which options are set regardless of their value. But this is much more complex to achieve for parsed json/yaml, so here we use pointers instead to determine whether a field is set. It's handy because their zero value is nil: without them it would be "" or 0 and we wouldn't be able to tell if it's defaulted or an actual value from the config file.

[moreirathomas]

Yeah obviously ! relying on nil is a great way of doing this !

[c1-ra]

Nice! It looks good to me!

[c1-ra]

Maybe you could just re-add the comment that Thomas suggested concerning Requests = -1 ?

[moreirathomas]

Nitpick-but-not-a-nitpick: explicit -1 results is the use of global timeout as exit condition. Omitting this option results in defaulting to -1 thus the global timeout.

[GregoryAlbouy]

Isn't it already implied given this comment is on defaultConfig?
On a side note we really need to rethink our defaults, I don't think infinite requests by default is a great idea, especially with a default timeout of 30s 🤔 100 would be more appropriate

[c1-ra]

I agree that a limited default value would actually be safer.