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.

Sign up for GitHub

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

Jump to bottom

Cleanup #22

Merged
merged 6 commits into from
Oct 12, 2017
Merged

Cleanup #22

merged 6 commits into from
Oct 12, 2017

Conversation

ericmann
Copy link
Contributor

Various cleanup changes:

  • Implement safe delete (E3DB-804)
  • Remove find by email (E3DB-805)
  • Privatize raw reads (E3DB-806)
  • Flatten the PublicKey interface (E3DB-807)

Eric Mann added 3 commits October 11, 2017 16:11
Quick SDK Updates
3448333 
* Support safe delete
* Unwrap public keys
Make raw reads private
d307c43
Remove share by email
f617a65
@coveralls
Copy link

coveralls commented Oct 11, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 61.029% when pulling f617a65 on cleanup into 380f565 on master.

gstro
gstro suggested changes Oct 12, 2017
View reviewed changes
Copy link

@gstro gstro left a comment

Choose a reason for hiding this comment

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

Need to update:

  • README instructions with wrapped key
  • registration_example.go with wrapped key
  • Use safe delete in cmd/e3db/main.go
  • Use safe delete in feedback.go

Maybe some others too?

cmd/e3db/main.go Outdated
@@ -354,7 +354,7 @@ func cmdDelete(cmd *cli.Cmd) {
client := options.getClient()

for _, recordID := range *recordIDs {
err := client.Delete(context.Background(), recordID)
err := client.Delete(context.Background(), recordID, "")
Copy link

Choose a reason for hiding this comment

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

Should actually use the safe delete, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the CLI, delete just takes a record ID ... unless we extend it to also require a version ID (which you'd have to somehow get through another method), then safe delete wouldn't work. For a simple CLI, skipping the safe delete feels OK, but I'm willing to discuss.

@coveralls
Copy link

coveralls commented Oct 12, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 61.029% when pulling c6dc65c on cleanup into 380f565 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 61.029% when pulling 4a263a4 on cleanup into 380f565 on master.

1 similar comment
@coveralls
Copy link

coveralls commented Oct 12, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 61.029% when pulling 4a263a4 on cleanup into 380f565 on master.

gstro
gstro approved these changes Oct 12, 2017
View reviewed changes
Copy link

@gstro gstro left a comment

Choose a reason for hiding this comment

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

¡LGTM! Thanks for the update!

@ericmann ericmann merged commit 9a34ecd into master Oct 12, 2017
@ericmann ericmann deleted the cleanup branch October 12, 2017 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gstro gstro gstro approved these changes

@m4dc4p m4dc4p Awaiting requested review from m4dc4p

@fernviridian fernviridian Awaiting requested review from fernviridian

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ericmann @coveralls @gstro