-
Notifications
You must be signed in to change notification settings - Fork 313
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
Show track and team revision when listing races #759
Show track and team revision when listing races #759
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one comment but otherwise it looks fine.
Can you please also set the milestone?
esrally/metrics.py
Outdated
@@ -1314,11 +1314,13 @@ def from_dict(cls, d): | |||
else: | |||
user_tags = {} | |||
|
|||
# Don't restore a few properties like cluster because they (a) cannot be reconstructed easily without knowledge of other modules | |||
team_revision = d.get("cluster").get("team-revision") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no cluster
property this will fail because #get()
will return None
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for catching this!
@@ -1160,11 +1160,11 @@ def format_dict(d): | |||
races = [] | |||
for race in race_store(cfg).list(): | |||
races.append([race.trial_id, time.to_iso8601(race.trial_timestamp), race.track, format_dict(race.track_params), race.challenge_name, race.car_name, | |||
format_dict(race.user_tags)]) | |||
format_dict(race.user_tags), race.track_revision, race.cluster.get("team-revision")]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi: I thought whether we might want to change the order here and group track-related properties and team-related properties but those two properties are so special that it's probably ok to leave them at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one more comment but feel free to merge after addressing my comment without another review round.
esrally/metrics.py
Outdated
@@ -1314,7 +1314,9 @@ def from_dict(cls, d): | |||
else: | |||
user_tags = {} | |||
|
|||
team_revision = d.get("cluster").get("team-revision") | |||
team_revision = None | |||
if d.get("cluster"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think more idiomatic is if "cluster" in d:
Show track and team revision when listing races Closes elastic#665
Closes #665