Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing tags flag format #619

Closed
wants to merge 1 commit into from
Closed

Conversation

luizbafilho
Copy link
Contributor

Changing separator from foo=bar to foo:bar in order to keep consistence
between how we format in flags like stages and environment variables,
since we use the envconfig project which also uses : as separator for maps

Changing separator from `foo=bar` to `foo:bar` in order to keep consistence
between how we format in flags like `stages` and environment variables,
since we use the envconfig project which also uses `:` as separator for maps
@luizbafilho luizbafilho requested a review from na-- May 10, 2018 17:21
@codecov-io
Copy link

Codecov Report

Merging #619 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #619      +/-   ##
==========================================
- Coverage   62.08%   62.07%   -0.02%     
==========================================
  Files          97       97              
  Lines        7512     7512              
==========================================
- Hits         4664     4663       -1     
- Misses       2588     2589       +1     
  Partials      260      260
Impacted Files Coverage Δ
cmd/options.go 37.63% <100%> (ø) ⬆️
js/modules/k6/ws/ws.go 71.3% <0%> (-0.43%) ⬇️
js/modules/k6/metrics/metrics.go 75% <0%> (ø) ⬆️

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 b62698d...131ceb9. Read the comment docs.

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the semantics here and with the stages are the same. This resembles the setting of environment variables more than the setting of stages, and that one uses = to separate keys and values.
Also, stages are not really keys and values like tags or environment variables, for the moment they are duration:vus pairs with the vus being optional. That would probably change and become more complicated when the arrival-rate executor is done, but it still won't be a key-value map.

@luizbafilho
Copy link
Contributor Author

I agree. But what is your suggestion then? should we keep the way it is now?

Which in the case is when using command-line flags it will be --tag foo=bar and environment variable K6_TAG=foo:bar

@na--
Copy link
Member

na-- commented May 11, 2018

Not sure to be honest. I personally don't like the format --tag foo:bar very much, but that's just a personal preference, so I'll be able to ignore it. On the other hand, besides personal taste, for environment variables I absolutely don't think it's worth it to do a breaking change so we can have --env foo:bar. And it doesn't make sense for one to use = and the other :.

Also, I don't think K6_TAG=foo:bar would be used very much, since you can specify only one tag that way. Other environment variables that are able to specify maps also seem like they would be rarely used. And, while I'll create a separate issue about my pain points with the current way options are processed, inherited and passed around, I'm not sure using envconfig would be wise in the long term. Not only because of this, other issues with the way we inconsistently treat options from different sources also play a role...

@luizbafilho
Copy link
Contributor Author

luizbafilho commented May 11, 2018

Actually, it won't be a breaking change because tags weren't released yet.

And regarding the setting of multiples tags with the environment variable envconfig supports it, you can just do K6_TAG=foo:bar,bob:alice

I would say that moving away from envconfig is a good call, but we have to make a decision whether using : or =. I'm ok with either. What're your thoughts on this @robingustafsson?

@na--
Copy link
Member

na-- commented May 11, 2018

Maybe I didn't phrase that correctly: it won't be a breaking change to do --tag foo:bar, but it would be a breaking change to do --env foo:bar. Environment variable setting via -e/--env has been released since 0.20. And it doesn't make sense to have one CLI flag with key:value and the other with key=value

@luizbafilho
Copy link
Contributor Author

Decided to wait for the config handling refactor before making any decisions on this, so we can pick one format and use it everywhere.

@luizbafilho luizbafilho deleted the fix/tags-flag-format branch May 22, 2018 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants