Support camelCase CSV output argument for file_name and save_interval. #2150

josephwoodward · 2021-10-01T00:26:43Z

This change adds support for camelCased versions of file_name and save_interval CSV output arguments,
writing a warning to the logs informing users the older snake cased argument will be deprecated in time.

This change adds support for camelCased versions of file_name and save_interval CSV output arguments, writing a warning to the logs informing users the older snake cased argument will be deprecated in time.

CLAassistant · 2021-10-01T00:26:47Z

All committers have signed the CLA.

codecov-commenter · 2021-10-01T08:40:08Z

Codecov Report

❗ No coverage uploaded for pull request base (master@75d30e6). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 2f396e2 differs from pull request most recent head 8307029. Consider uploading reports for the commit 8307029 to get more accurate results

@@            Coverage Diff            @@
##             master    #2150   +/-   ##
=========================================
  Coverage          ?   72.73%           
=========================================
  Files             ?      184           
  Lines             ?    14578           
  Branches          ?        0           
=========================================
  Hits              ?    10604           
  Misses            ?     3331           
  Partials          ?      643

Flag	Coverage Δ
ubuntu	`72.66% <0.00%> (?)`
windows	`72.37% <0.00%> (?)`

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

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 75d30e6...8307029. Read the comment docs.

josephwoodward · 2021-10-01T08:49:19Z

output/csv/config.go

@@ -59,7 +60,7 @@ func (c Config) Apply(cfg Config) Config {
 }

 // ParseArg takes an arg string and converts it to a config
-func ParseArg(arg string) (Config, error) {
+func ParseArg(arg string, logger *logrus.Logger) (Config, error) {


Do you have any thoughts on how we could avoid such a breaking change to the package's pubic API? Other options I was considering was:

Use Config to indicate a soon to be deprecated argument was used, and write the log message further up the call stack where the logger is initialised (in output.go)

Move the checking of the soon to be deprecated arguments to output.go

Hi, thanks for the PR!

Hhmm yeah, this is not ideal, but I think I prefer leaving it as is more than the alternatives, as currently it's clear that the warning happens where the configuration is assembled, and doing that validation outside of config.go would muddy things a bit.

Other than that a package like warnings might help, though it's not very idiomatic and we wouldn't want to add another dependency because of this. Maybe a custom error type that we can return and check for would be better.

So I'd say to leave it as is, unless someone from @grafana/k6-devs has a better idea.

Okay, thanks @imiric

Hhmm yeah, this is not ideal, but I think I prefer leaving it as is more than the alternatives, as currently it's clear that the warning happens where the configuration is assembled, and doing that validation outside of config.go would muddy things a bit.

Yeah, I agree - this was my rational, it keeps the logic local. Interested to see what @grafana/k6-devs think.

Perhaps, it makes sense to add this case (logging during config parsing) to #883 as "nice-to-have" to figure out how best to approach this if it comes up again?

@yorugac Good idea, I've just added this as a nice to have to that issue. 👍

josephwoodward · 2021-10-02T22:47:39Z

output/csv/config.go

@@ -92,7 +99,8 @@ func ParseArg(arg string) (Config, error) {

 // GetConsolidatedConfig combines {default config values + JSON config +
 // environment vars + arg config values}, and returns the final result.
-func GetConsolidatedConfig(jsonRawConf json.RawMessage, env map[string]string, arg string) (Config, error) {
+//nolint: lll


Passing the additional logger parameter causes the line length linting error, but given we're only passing the logger until people have migrated to the camel case version I felt the no lint comment would be sufficient for the time being.

I in general prefer to split the func <name>(<arguments>) (<results>){ on three lines as in

func <name>( <arguments>, )(<results>){

as it keeps it more readable especially when lines start to wrap.

mstoykov · 2021-10-04T13:49:02Z

output/csv/config_test.go

 	for arg, testCase := range cases {
 		arg := arg
 		testCase := testCase

 		t.Run(arg, func(t *testing.T) {
-			config, err := ParseArg(arg)
+			config, err := ParseArg(arg, testLog)


This could've also tested that the warning is being logged

output/csv/config.go

josephwoodward · 2021-10-04T15:03:11Z

@mstoykov Thank you for the comments, I will get these addressed this evening.

Co-authored-by: Mihail Stoykov <MStoykov@users.noreply.github.com>

josephwoodward · 2021-10-05T23:51:49Z

@mstoykov This is ready for a re-review when you can please. I've added an assertion over the expected log messages and level, and updated your other comments too.

inancgumus

LGTM

FloorD · 2021-10-06T13:20:16Z

Hey @josephwoodward! Thank you so much for your contribution! Because it's Hacktoberfest we would love to send you a k6 swag pack to celebrate. I'll reacxh out to you separately!

josephwoodward · 2021-10-06T13:22:08Z

@FloorD That'd be great! Big fan of what everyone is doing at K6/Grafana so certainly wouldn't say no!

Support camelCase csv output argument for file_name and save_interval.

518226a

This change adds support for camelCased versions of file_name and save_interval CSV output arguments, writing a warning to the logs informing users the older snake cased argument will be deprecated in time.

josephwoodward mentioned this pull request Oct 1, 2021

Fix snake_case to camelCase in argline for csv and influxdb outputs #2137

Closed

josephwoodward commented Oct 1, 2021

View reviewed changes

Address linting error

10d78a5

josephwoodward commented Oct 2, 2021

View reviewed changes

josephwoodward mentioned this pull request Oct 4, 2021

Configuration issues #883

Open

mstoykov reviewed Oct 4, 2021

View reviewed changes

output/csv/config.go Outdated Show resolved Hide resolved

mstoykov added this to the v0.35.0 milestone Oct 4, 2021

josephwoodward and others added 3 commits October 5, 2021 22:05

Update output/csv/config.go

4ed547f

Co-authored-by: Mihail Stoykov <MStoykov@users.noreply.github.com>

Correct csv output warning to indicate field is already deprecated

0e93149

Remove nolint comment in favour of formatting long function accordingly

ac736b0

josephwoodward force-pushed the 2137-camelCaseCsv branch 2 times, most recently from 69f2063 to db885e7 Compare October 5, 2021 23:45

Add test over csv output deprecation log warning

199248a

josephwoodward force-pushed the 2137-camelCaseCsv branch from db885e7 to 199248a Compare October 5, 2021 23:46

Add assertion to log level

8307029

inancgumus approved these changes Oct 6, 2021

View reviewed changes

mstoykov approved these changes Oct 6, 2021

View reviewed changes

mstoykov merged commit f76bfa5 into grafana:master Oct 6, 2021

josephwoodward deleted the 2137-camelCaseCsv branch October 6, 2021 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support camelCase CSV output argument for file_name and save_interval. #2150

Support camelCase CSV output argument for file_name and save_interval. #2150

josephwoodward commented Oct 1, 2021

CLAassistant commented Oct 1, 2021 •

edited

Loading

codecov-commenter commented Oct 1, 2021 •

edited

Loading

josephwoodward Oct 1, 2021

imiric Oct 1, 2021 •

edited

Loading

josephwoodward Oct 1, 2021 •

edited

Loading

yorugac Oct 4, 2021

josephwoodward Oct 4, 2021

josephwoodward Oct 2, 2021

mstoykov Oct 4, 2021

mstoykov Oct 4, 2021

josephwoodward commented Oct 4, 2021 •

edited

Loading

josephwoodward commented Oct 5, 2021

inancgumus left a comment

FloorD commented Oct 6, 2021

josephwoodward commented Oct 6, 2021

Support camelCase CSV output argument for file_name and save_interval. #2150

Support camelCase CSV output argument for file_name and save_interval. #2150

Conversation

josephwoodward commented Oct 1, 2021

CLAassistant commented Oct 1, 2021 • edited Loading

codecov-commenter commented Oct 1, 2021 • edited Loading

Codecov Report

josephwoodward Oct 1, 2021

Choose a reason for hiding this comment

imiric Oct 1, 2021 • edited Loading

Choose a reason for hiding this comment

josephwoodward Oct 1, 2021 • edited Loading

Choose a reason for hiding this comment

yorugac Oct 4, 2021

Choose a reason for hiding this comment

josephwoodward Oct 4, 2021

Choose a reason for hiding this comment

josephwoodward Oct 2, 2021

Choose a reason for hiding this comment

mstoykov Oct 4, 2021

Choose a reason for hiding this comment

mstoykov Oct 4, 2021

Choose a reason for hiding this comment

josephwoodward commented Oct 4, 2021 • edited Loading

josephwoodward commented Oct 5, 2021

inancgumus left a comment

Choose a reason for hiding this comment

FloorD commented Oct 6, 2021

josephwoodward commented Oct 6, 2021

CLAassistant commented Oct 1, 2021 •

edited

Loading

codecov-commenter commented Oct 1, 2021 •

edited

Loading

imiric Oct 1, 2021 •

edited

Loading

josephwoodward Oct 1, 2021 •

edited

Loading

josephwoodward commented Oct 4, 2021 •

edited

Loading