Skip to content

Commit

Permalink
fix(cmd): respect empty SummaryTrendStats value
Browse files Browse the repository at this point in the history
Resolves: b91229a#r35621359

This changes current behavior on `master`, but as discussed above, it makes
sense to respect whatever the user specifies, so this can be considered a fix.
  • Loading branch information
Ivan Mirić committed Oct 23, 2019
1 parent 638ffce commit 333ce28
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 13 deletions.
20 changes: 8 additions & 12 deletions cmd/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import (
"github.com/loadimpact/k6/stats/influxdb"
"github.com/loadimpact/k6/stats/kafka"
"github.com/loadimpact/k6/stats/statsd/common"
"github.com/loadimpact/k6/ui"
)

// configFlagSet returns a FlagSet with the default run configuration flags.
Expand Down Expand Up @@ -179,8 +178,15 @@ func readEnvConfig() (conf Config, err error) {
envconfig.Process("k6", &conf.Collectors.InfluxDB),
envconfig.Process("k6", &conf.Collectors.Kafka),
} {
// TODO(imiric): Remove this workaround once envconfig is updated to >=1.4.0, which
// would fix this issue: https://github.com/kelseyhightower/envconfig/pull/119
if len(conf.SummaryTrendStats) == 1 && conf.SummaryTrendStats[0] == "" {
conf.SummaryTrendStats = []string{}
}

return conf, err
}

return conf, nil
}

Expand Down Expand Up @@ -305,16 +311,6 @@ func getConsolidatedConfig(fs afero.Fs, cliConf Config, runner lib.Runner) (conf
conf = conf.Apply(envConf).Apply(cliConf)
conf = applyDefault(conf)

// TODO(imiric): Move this validation where it makes sense in the configuration
// refactor of #883. Currently we validate CLI flags in cmd.getOptions. Yet
// there is no place where configuration is validated after applying all other
// sources, like environment variables (besides inline in the runCmd itself...).
// So this repeats the trend stats validation in case other sources overrode our
// default value.
if err = ui.ValidateSummary(conf.SummaryTrendStats); err != nil {
return conf, err
}

return conf, nil
}

Expand All @@ -326,7 +322,7 @@ func applyDefault(conf Config) Config {
if conf.Options.SystemTags == nil {
conf = conf.Apply(Config{Options: lib.Options{SystemTags: lib.GetTagSet(lib.DefaultSystemTagList...)}})
}
if len(conf.Options.SummaryTrendStats) == 0 {
if conf.Options.SummaryTrendStats == nil {
conf = conf.Apply(Config{Options: lib.Options{SummaryTrendStats: lib.DefaultSummaryTrendStats}})
}
return conf
Expand Down
2 changes: 1 addition & 1 deletion cmd/config_consolidation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ func getConfigConsolidationTestCases() []configConsolidationTestCase {
assert.Equal(t, lib.DefaultSummaryTrendStats, c.Options.SummaryTrendStats)
}},
{opts{cli: []string{"--summary-trend-stats", `""`}}, exp{}, func(t *testing.T, c Config) {
assert.Equal(t, lib.DefaultSummaryTrendStats, c.Options.SummaryTrendStats)
assert.Equal(t, []string{}, c.Options.SummaryTrendStats)
}},
{
opts{runner: &lib.Options{SummaryTrendStats: []string{"avg", "p(90)", "count"}}},
Expand Down

0 comments on commit 333ce28

Please sign in to comment.