Implement call to move to highest supported REST API version #100

t8y8 · 2016-11-07T22:38:20Z

Implemented during TC16 Hackathon!

Today, by default we sign in as '2.3' hard coded as our API version.

Now users can call server.use_highest_version() and it will negotiate the highest-supported API version for the current Server.

t8y8 · 2016-11-08T02:04:36Z

tableauserverclient/server/server.py

+        response = self._session.get(self.server_address + "/auth?format=xml")
+        info_xml = ET.fromstring(response.content)
+        prod_version = info_xml.find('.//product_version').text
+        version = _PRODUCT_TO_REST_VERSION.get(prod_version, '2.1')  # 2.1


I'll make that comment not a random repetition: 2.1 is the version with permissions changes

Personally, I am not sure I would include this code. The implication of adding this is that we actually test against some of these older versions, which we don't. While believe we should work with most of it, my inclination would be that if the user asks us to pick the latest version, and we can't find it (ie there is no serverInfo endpoint), we set it to 9.3. I am open to discuss, though. I am sure it is helpful, however we do need to be clear about what we actually test against.

We could just default to 9.3's REST API version -- this was recommended by @RussTheAerialist since I think he plans to decorate versions back farther than 9.3.

I'm fine either way

graysonarts

Overall I like it.
Now the question is: should we have a flag on the server constructor that allows users to say "use server version" upon construction? @LGraber?

graysonarts · 2016-11-08T13:20:00Z

tableauserverclient/server/server.py

+        except ServerInfoEndpointNotFoundError:
+            version = self._get_legacy_version()
+
+        finally:


This seems a bit weird. We are setting the version, determining the new version, and then resetting the old version. I think with the way this is written, we probably don't need _determine_highest_version to be it's own function. In this case, we could do something like the following pseudo-code

save old version set version to 2.4 Try to call get rest api version If exception, try to get legacy version If no legacy version found, restore old value.

I'm not sure which version would be clearer to understand.

I currently have it this way so that this function simply retrieves the highest supported version, and the caller is responsible for setting it wherever it wants to be set -- this lets us reuse the function elsewhere (I don't know a use case yet, but I don't think the premature refactor makes it harder to follow).

I'd prefer the separation, if there's a strong objection let me know and I can combine it.

I'm pro-flag that does this automatically!!! I'll let Lee weigh in and can respond accordingly

I do not have a strong opinion either way, like I said, I wasn't sure which direction was clearer, and you make a good point about reuse.

Prefer the API is set to the highest level (since that is usually what most people expect). If someone wants to fix the version - they can set the version after sign in.

Why do you think if you put this in the use_highest_version() function it could not be used elsewhere? Just because it is a public function doesn't mean you can't call it from other parts of the code. Seems like we don't need two functions.

I think use_highest_version should do just that, find and set the highest version. get highest version, or do the best guess is something different, and I think it's cleaner to leave the setting to the caller (use_highest_version) and the internal machinery of how it finds that version are behind a different function and could change.

As a side benefit I think it keeps the little bits with the try/excepts much easier to follow to a new reader of the code, vs jamming it all into one function that also sets it on the Server object too

LGraber

I am waffling around on the server constructor. Generally, though, I prefer constructors to do a minimal amount of work. People tend to forget to put the first set of class declarations inside of try / catch blocks for one thing. The only way I see this as useful is if we set the default to be to use the latest version. Then the constructor flag would be to unset this so that you can explicitly set a value. Let's discuss in the office. Besides that, see my comments. Thanks for adding this!

LGraber · 2016-11-13T06:00:27Z

tableauserverclient/server/server.py

+        response = self._session.get(self.server_address + "/auth?format=xml")
+        info_xml = ET.fromstring(response.content)
+        prod_version = info_xml.find('.//product_version').text
+        version = _PRODUCT_TO_REST_VERSION.get(prod_version, '2.1')  # 2.1


Personally, I am not sure I would include this code. The implication of adding this is that we actually test against some of these older versions, which we don't. While believe we should work with most of it, my inclination would be that if the user asks us to pick the latest version, and we can't find it (ie there is no serverInfo endpoint), we set it to 9.3. I am open to discuss, though. I am sure it is helpful, however we do need to be clear about what we actually test against.

LGraber · 2016-11-13T06:04:07Z

tableauserverclient/server/server.py

+        except ServerInfoEndpointNotFoundError:
+            version = self._get_legacy_version()
+
+        finally:


Why do you think if you put this in the use_highest_version() function it could not be used elsewhere? Just because it is a public function doesn't mean you can't call it from other parts of the code. Seems like we don't need two functions.

t8y8 · 2016-11-13T07:29:55Z

@LGraber I don't follow your comments on the Server constructor -- I didn't change the behavior of __init__. We can chat more when I'm back to I can figure out what you mean (but remember I'm in 🗾 and 🇹🇼 for a while!)

LGraber · 2016-11-13T15:13:44Z

I was commenting on the question of whether we should add a flag to the constructor to do this versus having them always call the method. It is not actually in your code.

t8y8 · 2016-12-02T22:59:00Z

I just rebased on top of the latest changes -- I didn't make any code changes to the PR.

I think I saw these issues still open:

Default to API v2.2 instead of v2.1 per @LGraber
Why don't I put it all in use_highest_version <- I much prefer the code this way because it keeps the logic around version-detecting clean and keeps the try/except blocks out of the public function. I can move this if there's strong objection but I really don't want to
Should we have a flag somewhere to do this automatically? @LGraber and @RussTheAerialist both commented, no decisions.

* Jekyll Dependencies * First draft of docs -- we'll improve once skeleton is in place * Does not deploy yet, need to do that in github settings page

t8y8 force-pushed the 99-feature-detect-version branch 2 times, most recently from c93fea4 to 266bef4 Compare November 8, 2016 01:57

t8y8 commented Nov 8, 2016

View reviewed changes

graysonarts reviewed Nov 8, 2016

View reviewed changes

LGraber reviewed Nov 13, 2016

View reviewed changes

t8y8 force-pushed the 99-feature-detect-version branch from 266bef4 to 3a0583a Compare December 2, 2016 22:56

t8y8 mentioned this pull request Dec 14, 2016

Issue 112 - Support for the new image api endpoint #113

Merged

graysonarts approved these changes Jan 5, 2017

View reviewed changes

Implement call to move to highest supported REST API version

6150175

t8y8 force-pushed the 99-feature-detect-version branch from 3a0583a to 6150175 Compare January 5, 2017 21:30

t8y8 merged commit bfff665 into tableau:development Jan 5, 2017

t8y8 deleted the 99-feature-detect-version branch January 5, 2017 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement call to move to highest supported REST API version #100

Implement call to move to highest supported REST API version #100

t8y8 commented Nov 7, 2016

t8y8 Nov 8, 2016

LGraber Nov 13, 2016

t8y8 Nov 13, 2016 •

edited

graysonarts left a comment

graysonarts Nov 8, 2016

t8y8 Nov 8, 2016 •

edited

graysonarts Nov 8, 2016

divyavanmahajan Nov 11, 2016

LGraber Nov 13, 2016

t8y8 Nov 13, 2016 •

edited

LGraber left a comment

LGraber Nov 13, 2016

LGraber Nov 13, 2016

t8y8 commented Nov 13, 2016 •

edited

LGraber commented Nov 13, 2016

t8y8 commented Dec 2, 2016 •

edited

Implement call to move to highest supported REST API version #100

Implement call to move to highest supported REST API version #100

Conversation

t8y8 commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t8y8 Nov 13, 2016 • edited

Choose a reason for hiding this comment

graysonarts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t8y8 Nov 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t8y8 Nov 13, 2016 • edited

Choose a reason for hiding this comment

LGraber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t8y8 commented Nov 13, 2016 • edited

LGraber commented Nov 13, 2016

t8y8 commented Dec 2, 2016 • edited

t8y8 Nov 13, 2016 •

edited

t8y8 Nov 8, 2016 •

edited

t8y8 Nov 13, 2016 •

edited

t8y8 commented Nov 13, 2016 •

edited

t8y8 commented Dec 2, 2016 •

edited