-
Notifications
You must be signed in to change notification settings - Fork 23
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
Improve messages for tasks/jobs #897
Conversation
@@ -164,7 +164,7 @@ def run(self): | |||
NS.publisher_id, | |||
{"message": "Please wait while Tendrl imports (" | |||
"job_id: %s) newly expanded " | |||
"%s storage nodes %s" % ( | |||
"%s storage nodes in cluster %s" % ( | |||
_job_id, | |||
sds_pkg_name, | |||
integration_id)}, |
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.
please use cluster.short_name or cluster.integration_id
in all messages
rather, just write a function cluster.to_str
which will return either cluster.short_name or cluster.integration_id
@@ -15,7 +15,7 @@ def expand_gluster(parameters): | |||
logger.log( | |||
"info", | |||
NS.publisher_id, | |||
{"message": "Setting up gluster nodes %s" % | |||
{"message": "Setting up gluster nodes for cluster %s" % | |||
parameters['TendrlContext.integration_id']}, |
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.
please use cluster.short_name or cluster.integration_id in all messages
{"message": "Expanded Gluster Cluster %s." | ||
" New nodes are: %s" % ( | ||
{"message": "Expanded Gluster Cluster %s" | ||
" with nodes %s" % ( | ||
parameters['TendrlContext.integration_id'], |
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.
please use cluster.short_name or cluster.integration_id in all messages
"to cluster %s" % ( | ||
"message": "ImportCluster %s (jobID: %s) : " | ||
"importing host %s" % ( | ||
integration_id, |
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.
please use cluster.short_name or cluster.integration_id in all messages
Signed-off-by: Nishanth Thomas <nthomas@redhat.com>
Signed-off-by: Nishanth Thomas <nthomas@redhat.com>
Signed-off-by: Nishanth Thomas <nthomas@redhat.com>
@@ -90,7 +90,7 @@ def process_job(job): | |||
# "failed" (the parent job of these jobs will also be | |||
# marked as "failed") | |||
if "tendrl/monitor" in NS.node_context.tags and \ | |||
_timeout == "yes": | |||
_timeout == "yes": |
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.
Do we need these spaces? I feel no.
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.
This was to resolve the pep8 issue
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.
Ack
"imported %s" % (node_list, integration_id)}, | ||
{"message": "ImportCluster %s waiting for hosts %s " | ||
"to be " | ||
"imported" % (_cluster.short_name, node_list)}, |
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.
This line also should be aligned with previous one
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.
done
Signed-off-by: Nishanth Thomas <nthomas@redhat.com>
LGTM |
tendrl/commons/jobs/__init__.py
Outdated
{"message": "Running Flow %s" % | ||
job.payload['run']}, | ||
{"message": "Running %s" % | ||
job.payload['run'].rpartition('.')[2]}, |
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.
please use simple python code, what is the guarantee of rpartition and index 2 being always present? This code is going to fail as soon as we add one more layer of heirarchy to "NS.tendrl.flows.ImportCluster"
job.payload['run'].split(".")[-1]
itendrl-bug-id: /issues/834