-
Notifications
You must be signed in to change notification settings - Fork 217
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
Fix incorrect print commands in spreaper #1299
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.
Some requested changes...
reply = reaper.post("snapshot/cluster/{0}".format(args.cluster_name), | ||
keyspace=args.keyspace, | ||
owner=args.owner, cause=args.cause, | ||
snapshot_name=args.name) | ||
else: | ||
if args.node: | ||
print ("# Taking a snapshot on cluster '{0}' and node '{1}', " | ||
).format(args.cluster_name, args.node) | ||
.format(args.cluster_name, args.node)) | ||
reply = reaper.post("snapshot/{0}/{1}".format(args.cluster_name, args.node), | ||
keyspace=args.keyspace, | ||
owner=args.owner, cause=args.cause, | ||
snapshot_name=args.name) | ||
else: | ||
print ("# Taking a snapshot on cluster '{0}', " |
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.
Extra comma here.
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 prints
# Taking a snapshot on cluster 'cname',
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.
👍
src/packaging/bin/spreaper
Outdated
@@ -638,11 +638,11 @@ class ReaperCLI(object): | |||
|
|||
if args.node: | |||
print ("# Deleting snapshot '{1}' on cluster '{0}' and node '{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 check the ordering of arguments for this one. I think you've reversed cluster name and snapshot name.
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.
right 👍
src/packaging/bin/spreaper
Outdated
"targeting tables: {2}").format(args.cluster_name, args.keyspace_name, | ||
",".join(args.tables.split(','))) | ||
"targeting tables: {2}".format(args.cluster_name, args.keyspace_name, | ||
",".join(args.tables.split(',')))) |
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.
Why do you split and then rejoin? Seems redundant.
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.
The hell if I know 😅 so weird...
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.
Looks better now!
Fixes #1282
The print commands in spreaper's
take-snapshot
method (and a few others) was not built correctly and had misplaced closing parenthesis.