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

Add kv rollback #4774

Merged
merged 3 commits into from
Jun 15, 2018
Merged

Add kv rollback #4774

merged 3 commits into from
Jun 15, 2018

Conversation

jefferai
Copy link
Member

Like kv patch this is more of a helper than anything else; it provides
a single command to fetch the current version (for CAS), read the
version you want to roll back to, and set it as the new version (using
CAS for safety).

Like `kv patch` this is more of a helper than anything else; it provides
a single command to fetch the current version (for CAS), read the
version you want to roll back to, and set it as the new version (using
CAS for safety).
@jefferai jefferai added this to the 0.10.3 milestone Jun 15, 2018
@jefferai jefferai requested a review from briankassouf June 15, 2018 11:33

// Now run it again and read the version we want to roll back to
var data map[string]interface{}
{
Copy link
Contributor

Choose a reason for hiding this comment

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

never seen this syntax before, do it create a new lexical scope?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Here it's just for logical separation so I could use the same mechanism but not worry about declaration vs assignment.

Copy link
Contributor

@jippi jippi Jun 15, 2018

Choose a reason for hiding this comment

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

thats a nice trick, the lazy version of a func

Copy link
Member Author

Choose a reason for hiding this comment

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

There's enough difference that a function having to check parameters would have ended up as long, and quite possibly more complicated.

return 2
}

// Verify current data found
Copy link
Contributor

@briankassouf briankassouf Jun 15, 2018

Choose a reason for hiding this comment

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

I'm not sure this step is necessary. If the version has been marked deleted there wouldn't be any data returned, but we should still allow a rollback on that key.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

}

if meta["deletion_time"] != nil && meta["deletion_time"].(string) != "" {
c.UI.Error(fmt.Sprintf("Cannot roll back to a version that has been deleted"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Were you able to test if this check works? I think if the version has been deleted then no data would be returned and the above data checks would fail first.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought if it was deleted it 404s but still returns metadata. I'll check.

}

if meta["destroyed"] != nil && meta["destroyed"].(bool) {
c.UI.Error(fmt.Sprintf("Cannot roll back to a version that has been destroyed"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor

@briankassouf briankassouf left a comment

Choose a reason for hiding this comment

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

Looks good!

@jefferai jefferai merged commit 9dd25aa into master Jun 15, 2018
@jefferai jefferai deleted the kv-rollback branch June 15, 2018 19:34
@jefferai jefferai mentioned this pull request Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants