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

Versioning #17

Open
blitzmann opened this issue Oct 17, 2015 · 10 comments
Open

Versioning #17

blitzmann opened this issue Oct 17, 2015 · 10 comments
Milestone

Comments

@blitzmann
Copy link
Contributor

Per the (documentation)[http://eveonline-third-party-documentation.readthedocs.org/en/latest/crest/versioning/], versioning should be supported on a per-resource basis. Right now you can modify the headers when you create the connection, but not when you call a resource.

@jonobrien
Copy link
Contributor

#39 seems to have started on this

@hkraal
Copy link
Contributor

hkraal commented Jul 27, 2016

#39 is about the feedback, next step is creating an header when doing:

>>> eve(version=4)

This will be a challenge however as we may or may not have the required information at that point to create the application/vnd.ccp.eve.Api-v5+json string (the eve.Api is dynamic)

For now one could inject the headers I suppose, at least you can have a clue about which version you're getting served by CREST

@jonobrien
Copy link
Contributor

not all endpoints support all versions either

@hkraal
Copy link
Contributor

hkraal commented Jul 29, 2016

@jonobrien do you have an example endpoint were that's the case?

@Kyria
Copy link
Contributor

Kyria commented Jul 29, 2016

I'm wondering if I didn't had this issue one time, with the market order endpoints... But i can't find any trace in my commits, so it must have been on my dev env...
But if remember well, the main issue I had was when I forced an API version (in HTTP Accept header) in the init of ApiConnection, is that some endpoints were having lesser version (as latest) or the contrary, the version given was considered as deprecated / removed, so I had to force version each time I wanted to get something not to get some errors from CREST.

I think the best way to do it here is to set no HTTP Accept, so it will always pull the latest version, unless the user manually specify it when he call the get method on APIConnection.
This way, the user/dev have the choice to set it each time he really need a specific version, without the risk of breaking every other endpoints due to outdated version or nonexistent version, or simply use the latest version of his endpoint if he doesn't care about it.

@hkraal
Copy link
Contributor

hkraal commented Jul 29, 2016

Hmmm @jonobrien I think I misunderstood you before. You mean I can't ask for eve(version=1) because that one isn't supported anymore? I would assume this should raise some sort of error which should be sent to the user.

@wtfrank is working on the APIException to return additional information which it got back from CREST. This (requesting a invalid version endpoint) is a case which should be taken into account for that I suppose.

@Kyria
I concur the default should always request the latest endpoint version. This is also the most logical thing to do as we don't "know" what the latest endpoint version actually is.

@jonobrien
Copy link
Contributor

@hkraal check out some schemas at crestmatic there are the json schemas of some endpoints with the api version they support, not sure if all endpoints are available for all versions, as you clarified for me. I have not tried querying with an older header to an older endpoint, it may still work, it may not.

@Kyria this works unless we don't handle an endpoint change CCP makes and we become broken after a change to CREST. There is v5 right now, I believe is the latest(?) and we are working fine on that.

@jonobrien
Copy link
Contributor

if we dynamically walk endpoints it actually probably doesn't matter if we just request latest version anyway, so having default be latest, flag for certain version might be a good way of handling things.

@wtfrank
Copy link
Contributor

wtfrank commented Jul 29, 2016

I agree that the sensible thing to do is not request any endpoint version unless the user requests one. If the user requests version 57 (which doesn't exist) then we should just send that in the header to CCP and allow them to return whatever error they want. So a really dumb piece of code is all that's needed: if version_specified then set_header(the_version_that_was_specified)

so the documentation of the feature will be bigger than the implementation of it :)

@wtfrank
Copy link
Contributor

wtfrank commented Jul 29, 2016

I would just write it in 5 minutes and make a pull request, but I should resist the urge and wait until the cache info change finds its way into main so I can test it properly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants