-
Notifications
You must be signed in to change notification settings - Fork 50
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
bug: Add try/except handler for force #1535
Conversation
This will wrap the force `delete_service_data` with a try/except handler so as not to prevent the tokenserver record from being removed if the delete_service_data fails.
* Try/except will capture any errors from trying to call the delete on a given node, this could fail if the auth or node aren't correct. * Override will override the attempted node. This will allow us to force the delete message, even if the original data was copied over and the node was incorrect.
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.
LGTM, fully acknowledging my relative unfamiliarity with tokenserver. Just a few small tweaks here and there. Thanks for the formatting run, it makes it easier to read for sure!
), | ||
}, | ||
secret=secret, | ||
) | ||
secret = tokenlib.get_derived_secret(token, secret=secret) | ||
endpoint = PATTERN.format(uid=user.uid, node=user.node) | ||
auth = HawkAuth(token, secret) |
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.
Is there a reason we don't do the dryrun check sooner in the function? Just wondering if it's better to exit earlier before making the token.
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.
Mostly just to exercise code. For a dry run, I prefer doing as much as possible up until I'm about to do something destructive / permanent so that I know that all other aspects/functions work. Ideally, the dryrun
would dump state so that could see what the action to be taken would be, but that can produce a lot of noise.
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.
Ah that's a great point!
…torage-rs into bug/SYNC-3426_try
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.
🚀 👍
Description
This will wrap the force
delete_service_data
with a try/except handlerso as not to prevent the tokenserver record from being removed if the
delete_service_data fails.
override_node
option to attempt the delete using the specified node instead of the recorded one. (useful if the original record was copied over and the subsequent managing node changed.)black
to improve formattingIssue(s)
Closes SYNC-3426