add --user-tags as alias for --user-tag#1599
Conversation
pakio
force-pushed
the
ISSUE-1577_add-user-tag-alias
branch
2 times, most recently
from
447e7e0 to
277c303
Compare
pakio
force-pushed
the
ISSUE-1577_add-user-tag-alias
branch
from
277c303 to
5887733
Compare
| "--user-tag", | ||
| "--user-tags", |
There was a problem hiding this comment.
didn't change the order to keep backward compatibility, in order to be able to set tag by env var USER_TAG.
dliappis
left a comment
dliappis
left a comment
There was a problem hiding this comment.
Thank you for raising this!
I left some comments. One thing that is missing IMHO is that we should emit a "Deprecation warning" when --user-tag is use in the CLI. That way people who don't happen to check the migration guide or read docs will be reminded that they should be switching away.
docs/command_line_reference.rst
Outdated
|
|
||
|
|
||
| When you run ``esrally list races``, this will show up again:: | ||
| Wesrally list raceshen you run ``esrally list races``, this will show up again:: |
There was a problem hiding this comment.
I think this was introduced by accident?
There was a problem hiding this comment.
unintended, thank you for the catch 🙏
docs/metrics.rst
Outdated
| * Source revision: We always record the git hash of the version of Elasticsearch that is benchmarked. This is even done if you benchmark an official binary release. | ||
| * Distribution version: We always record the distribution version of Elasticsearch that is benchmarked. This is even done if you benchmark a source release. | ||
| * Custom tag: You can define one custom tag with the command line flag ``--user-tag``. The tag is prefixed by ``tag_`` in order to avoid accidental clashes with Rally internal tags. | ||
| * Custom tags: You can define one custom tags with the command line flag ``--user-tags``. The tags are prefixed by ``tag_`` in order to avoid accidental clashes with Rally internal tags. |
There was a problem hiding this comment.
| * Custom tags: You can define one custom tags with the command line flag ``--user-tags``. The tags are prefixed by ``tag_`` in order to avoid accidental clashes with Rally internal tags. | |
| * Custom tags: You can define custom tags with the command line flag ``--user-tags``. The tags are prefixed by ``tag_`` in order to avoid accidental clashes with Rally internal tags. |
docs/migrate.rst
Outdated
| for lap in $(seq 1 ${RALLY_LAPS}) | ||
| do | ||
| esrally --pipeline=benchmark-only --user-tag lap:$lap | ||
| esrally --pipeline=benchmark-only --user-tags lap:$lap |
There was a problem hiding this comment.
It would be very nice if we could document at the top of this file that --user-tag is now deprecated in favor of --user-tags. The next version can be found in https://github.com/elastic/rally/blob/master/esrally/_version.py#L1 so it'll be effective from 2.6.1.
There was a problem hiding this comment.
As mentioned in #1599 (comment) maybe it's just enough to mention that starting with 2.6.1 --user-tags has been introduced as a "synonym" to --user-tag.
There was a problem hiding this comment.
oh, sorry I didn't notice this was a migration guide.
I think that might be better to be in the migration guide section for v2.6.1, so I simply reverts this change.
dliappis
added
:Config
I discussed with @pquentin and instead of doing what I proposed here, we could just keep the functionality for both i.e. no need for a depreciation warning and allow both for the foreseeable future. Given the large usage of Rally this would probably be the least disruptive change for users. |
|
Thank you @dliappis for your discussion and review! Given the result, do you think the current way of migration, adding |
|
@elasticmachine test this please |
dliappis
left a comment
dliappis
left a comment
There was a problem hiding this comment.
Sorry for the delay, this looks great thank you. I've triggered now CI tests, if they are green, I'll merge it in.
|
@elasticmachine test this please |
3 participants
This PR closes #1577, adding a track option
--user-tagsas an alias for--user-tag.==== checklist ====
make check-allsuccessfully? - yes