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

Allow connecting to Kuzzle v2 #260

Merged
merged 4 commits into from
Oct 12, 2020
Merged

Allow connecting to Kuzzle v2 #260

merged 4 commits into from
Oct 12, 2020

Conversation

tsauvajon
Copy link
Contributor

What does this PR do?

This PR allows an application using this SDK to connect to a Kuzzle server running on the latest version.

Several changes had to be made:

How should this be manually tested?

Follow the examples at https://docs.kuzzle.io/sdk/go/1/essentials/getting-started/ to connect to a Kuzzle server and create documents.

This should be tested in an environment that doesn't use a cached github.com/satori/go.uuid: either using Go Modules, or by updating/removing the dependency.

Boyscout

Fixed the dependencies versions using Go Modules so dependency changes don't break things again.

@tsauvajon
Copy link
Contributor Author

The Travis tests fail because the dependencies are cached

@Aschen
Copy link
Contributor

Aschen commented Jun 30, 2020

Thanks you for your pr @tsauvajon !

Actually releasing the SDK for Kuzzle v2 is gonna take a little bit more time since we have to:

  • creates new 3-dev and 2-stable branches
  • updates the release tool

Also to be fully compatible with Kuzzle v2, we need to change some methods according to this guide: https://docs.kuzzle.io/core/2/guides/upgrade-kuzzle/changes/
(looks this PR on the Dart SDK for example)

I'm gonna change the target branch for this PR to 3-dev

@Aschen Aschen changed the base branch from master to 3-dev June 30, 2020 08:39
@Aschen Aschen self-assigned this Jun 30, 2020
@Aschen Aschen changed the title changelog: Allow connecting to Kuzzle v2 Allow connecting to Kuzzle v2 Jun 30, 2020
@scottinet
Copy link
Contributor

We need to overhaul this entire SDK. Probably restarting it from scratch. So many things changed in our practices and SDK APIs that I don't see how we could benefit from building upon this old one. 🤔

@Aschen
Copy link
Contributor

Aschen commented Jul 1, 2020

So many things changed in our practices and SDK APIs

Can you be more precise? Because rewriting a SDK from scratch is gonna take time so if we decide to do it we have to be sure that's it's worth it.
I'm saying this because the Go SDK is not the most used one

@scottinet
Copy link
Contributor

From the top of my head: how options are passed to functions by users, the fact that a lot of code in there was to be used with swig, the whole API that is somewhere between our SDK JS 5 and 6, and probably more.

@tsauvajon
Copy link
Contributor Author

tsauvajon commented Jul 1, 2020

From the top of my head: how options are passed to functions by users, the fact that a lot of code in there was to be used with swig, the whole API that is somewhere between our SDK JS 5 and 6, and probably more.

I can add a few things too. There are many cases where you can reach a deadlock, and there are race conditions. Both can be fixed without rewriting the whole thing, though.

I wrote a quick test with 1 subscription and a loop adding documents. I ran my test about 20 times and it was never able to get past 80 messages before reaching a deadlock. That makes me think the SDK is probably not used by many people, otherwise they would have complained about this before.

@Aschen
Copy link
Contributor

Aschen commented Oct 12, 2020

Hi @tsauvajon !

We just had an internal discussion and we are gonna merge your contribution, thanks again :-)

Rewriting the whole SDK would had been too long and we are aware that not many people are using it.
For now we are just gonna release a new version for Kuzzle v2 and later we will think about a complete rewrite to address the issues you talked about.

@Aschen Aschen merged commit ef06f4e into kuzzleio:3-dev Oct 12, 2020
@Yoann-Abbes Yoann-Abbes mentioned this pull request Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants