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

work towards fixing dht commands #6277

Merged
merged 4 commits into from
May 14, 2019
Merged

work towards fixing dht commands #6277

merged 4 commits into from
May 14, 2019
+52 −30

Conversation

Stebalien
Copy link
Member

Specifically, #3124

Unfortunately, dht put is still broken. However, I can confirm that this fixes
dht get.

@Stebalien
commands(dht): make it possible to return errors from printEvent
76dc6f5 
License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien
commands(dht): base64 encode value in get
9027eaa 
Otherwise, it seems that something is treating this as UTF8 and normalizing it?

fix half of #3124

License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien
commands(dht): load arguments from request body
cc2ab14 
License: MIT
Signed-off-by: Steven Allen <[email protected]>
@ghost ghost assigned Stebalien Apr 30, 2019
@ghost ghost added the status/in-progress In progress label Apr 30, 2019
@Stebalien
test: dht put/get round trips
a8fa9dd 
License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien Stebalien marked this pull request as ready for review May 14, 2019 19:05
@Stebalien Stebalien requested a review from djdv May 14, 2019 19:06
@Stebalien
Copy link
Member Author

That issue isn't completely fixed but this is still an improvement.

djdv
djdv approved these changes May 14, 2019
View reviewed changes
Copy link
Contributor

@djdv djdv left a comment

Choose a reason for hiding this comment

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

Nothing stands out. For a partial fix, looks good to me.

@Stebalien
Copy link
Member Author

Stebalien commented May 14, 2019 •
edited
Loading

Well, I guess I forgot to mention: this is a breaking change! We're now base64 encoding values returned by dht get when encoding them into JSON. However, there's nothing much we can do about that as DHT records are binary and we need to encode them somehow. Given that this API was fundamentally broken, :shipit:.

@Stebalien Stebalien merged commit 307d06b into master May 14, 2019
@Stebalien Stebalien deleted the fix/3124 branch May 14, 2019 19:38
@djdv
Copy link
Contributor

djdv commented May 14, 2019

We're now base64 encoding values returned by dht get when encoding them into JSON.

Probably useful for the release notes.
(Explicit back reference for later)
#6305

@djdv djdv mentioned this pull request May 23, 2019
Merged
@Stebalien Stebalien restored the fix/3124 branch May 30, 2019 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djdv djdv djdv approved these changes

Assignees

@Stebalien Stebalien

Labels
status/in-progress In progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Stebalien @djdv