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

mgr: dashboard code cleanup #15577

Merged
merged 4 commits into from Jun 8, 2017
Merged

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented Jun 8, 2017

No description provided.

// into "1.2.3-g9asdasd"
return /ceph version (.+) \(.+\)$/.exec(version)[1];
// Expect "ceph version 1.2.3-g9asdasd (as98d7a0s8d7)"
result = /ceph version (.+) \(.+\)/.exec(version);
Copy link

Choose a reason for hiding this comment

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

Nit: you should use /ceph version ([^ ]+) \(.+\)/

Copy link
Member

Choose a reason for hiding this comment

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

can we make a structured ceph version thing? i was considering changing the format of this string a couple weeks ago and didn't out of fear of things like this. but we have 2 new fields added at the end now (release name, e.g. 'luminous') and release type ('dev', 'rc', 'stable').

Copy link
Member

Choose a reason for hiding this comment

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

(that can come later, doesn't need to hold up this pr)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The places we pass it around (e.g. daemon metadata) we only have a string.

We could improve the format of the string though -- prefixing with "ceph version" seems redundant. Once that's removed we could guide people to take everything up to the first space as the short version and treat everything else as detail, perhaps.

@dillaman regex updated

@@ -577,6 +577,9 @@ def servers(self):
)

def _servers(self):
log.info("Listing servers...")
Copy link

Choose a reason for hiding this comment

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

Nit: debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

John Spray added 4 commits June 8, 2017 11:43
...so that classes that need one aren't creating
their own all the time.

Signed-off-by: John Spray <john.spray@redhat.com>
Signed-off-by: John Spray <john.spray@redhat.com>
Signed-off-by: John Spray <john.spray@redhat.com>
This was a hangover from when these wrapper classes
were borrowed from Calamari, which used these
versions/equality functions to work out when
to go fetch data from the ceph cluster.

Signed-off-by: John Spray <john.spray@redhat.com>
Copy link

@dillaman dillaman left a comment

Choose a reason for hiding this comment

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

lgtm

@dillaman dillaman merged commit 108b38e into ceph:master Jun 8, 2017
@jcsp jcsp deleted the wip-dashboard-cleanup branch November 7, 2017 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants