Add connection credentials by geordielad · Pull Request #80 · tableau/server-client-python

geordielad · 2016-10-26T23:19:20Z

No description provided.

graysonarts · 2016-10-27T16:34:52Z

Something about your changes are breaking tests to a point where they cannot run at all. Also, there is a merge conflict because I believe you based your changes on master and not development.

To resolve this in your fork, can you do the following:

git checkout master
git checkout -b development
git pull origin development
git checkout Add-Connection-Credentials # or whatever your branch name is locally
git rebase development

This last command will retarget your changes against development, but will probably require you to address merge conflicts along the way. If you aren't familiar with git, it can be daunting. https://help.github.com/articles/resolving-merge-conflicts-after-a-git-rebase/ provides a brief overview of how to fix the merge conflicts that may happen.

Then you'll need to repush your branch to github. To do this you will need to do git push --force origin Add-Connection-Credentials in order to update the PR.

Let me know if you have any questions.

…ient-python into development

geordielad · 2016-10-27T17:22:03Z

I think I have fixed the conflict. Do I need to do a new pull request now?

geordielad · 2016-10-27T17:23:57Z

Commenting seemed to closed the pull request Lets try that again.

graysonarts · 2016-10-27T17:59:03Z

Robin, looks like there is still a merge conflict. Did you rebase against development? that should have allowed you to fix the conflict for it to work in Github.

You don't need to create a new PR, the existing PR gets updated with your new commits when you push to your fork's branch.

Allow users to add connection credentials to Datasources and workbook publish requests. Should work for smaller requests and chunked requests

geordielad · 2016-10-27T18:10:15Z

Last time I aborted the rebase. Now I rand the rebase steps. fixed the conflict, added the changed file then did rebase continue then the git push

geordielad · 2016-10-27T18:13:04Z

I see the tests failing because of coding style. Lets see if I can fix that

The command "python setup.py test" exited with 0. 1.27s$ pycodestyle . ./tableauserverclient/__init__.py:10:26: W292 no newline at end of file ./tableauserverclient/server/request_factory.py:40:67: E231 missing whitespace after ',' ./tableauserverclient/server/request_factory.py:268:42: E231 missing whitespace after ',' ./tableauserverclient/server/request_factory.py:277:65: E231 missing whitespace after ',' ./tableauserverclient/server/endpoint/datasources_endpoint.py:125:121: E501 line too long (126 > 120 characters) ./tableauserverclient/server/endpoint/workbooks_endpoint.py:174:121: E501 line too long (122 > 120 characters)

graysonarts · 2016-10-27T18:28:08Z

Rad! Looks like it can merge correctly! Thanks! I'll take a look right now

graysonarts

Straight forward implementation. We'll need to add some tests before release next week, but should be easy enough to tackle. Thanks! 🚀

graysonarts · 2016-10-27T18:29:56Z

        datasource_element.attrib['name'] = datasource_item.name
        project_element = ET.SubElement(datasource_element, 'project')
        project_element.attrib['id'] = datasource_item.project_id
+        if connection_credentials:


(not a shipblocker)
Since this is common code, we might consider pulling this into its own helper method.

t8y8

A few minor things, and then can you add a test that covers this element?

I think a test in test_workbooks and do a publish where you check that this serialized properly would be fine.

EDIT: @RussTheAerialist said tests can come from a different PR, I'm OK with that (since we don't have serializer tests, I think I can actually do that PR, I have an idea)

t8y8 · 2016-10-27T19:02:07Z

+            credentials_element = ET.SubElement(workbook_element, 'connectionCredentials')
+            credentials_element.attrib['name'] = connection_credentials.name
+            credentials_element.attrib['password'] = connection_credentials.password
+            credentials_element.attrib['embed'] = str(connection_credentials.embed).lower()


Can you do a more explicit assignment instead of relying on str(True).lower() == 'true'?

credentials_element.attrib['embed'] = 'true' if connection_credentials.embed else 'false'

Or even more explicit

if connection_credentials.embed: credentials_element.attrib['embed'] = 'true' else: credentials_element.attrib['embed'] = 'false'

t8y8 · 2016-10-27T19:03:00Z

@@ -0,0 +1,5 @@
+class ConnectionCredentials(object):
+    def __init__(self, name, password, embed=True):
+        self.password = password


Nit: Reorder these to match the order in the function signature

if I fix order of property set in credentials do you want me to fix auth while I am doing it as I copied the code for this class from auth. New code would be

class TableauAuth(object): def __init__(self, username, password, site='', user_id_to_impersonate=None): self.username = username self.password = password self.site = site self.user_id_to_impersonate = user_id_to_impersonate

t8y8 · 2016-10-27T19:05:16Z

@@ -0,0 +1,5 @@
+class ConnectionCredentials(object):


Can you add a docstring on this class that tells users they should delete this object when they're done using it?

Explicit convert of boolean to true/false string Ordering of class properties to match signature Docstring on connection credentials and auth classes

t8y8 · 2016-10-27T20:08:48Z

+
+	"""
    def __init__(self, name, password, embed=True):
+       	self.name = name


indenting got all messed up here -- maybe it's tabs instead of 4 spaces?

t8y8 · 2016-10-27T20:32:35Z

Almost there @geordielad

./tableauserverclient/models/connection_credentials.py:4:1: W191 indentation contains tabs
./tableauserverclient/models/connection_credentials.py:4:67: W291 trailing whitespace
./tableauserverclient/models/connection_credentials.py:5:1: W191 indentation contains tabs
./tableauserverclient/models/connection_credentials.py:7:1: W191 indentation contains tabs
./tableauserverclient/models/connection_credentials.py:8:5: E901 IndentationError: unindent does not match any outer indentation level

Looks like your editor is using tabs instead of 4 spaces -- if using Sublime you can fix that in View > Indentation > Convert Tabs to Spaces

t8y8 · 2016-10-27T20:49:00Z

+    """Connection Credentials for Workbooks and Datasources publish request.
+
+    Consider removing this object and other variables holding secrets
+    as soon as poobible after use to avoid them hanging around in memory.


t8y8 · 2016-10-27T21:01:07Z

🚀

t8y8 · 2016-10-27T21:05:16Z

Nah, it's ok to limit your PR to just your code for now. This is more
preventative maintenance... That'll get cleaned up the next time we visit
it :)

On Oct 27, 2016 12:22, "geordielad" notifications@github.com wrote:

@geordielad commented on this pull request.

In tableauserverclient/models/connection_credentials.py
#80:
@@ -0,0 +1,5 @@
+class ConnectionCredentials(object):
def init(self, name, password, embed=True):
   self.password = password
if I fix order of property set in credentials do you want me to fix auth
while I am doing it as I copied the code for this class from auth. New code
would be

class TableauAuth(object):
def init(self, username, password, site='',
user_id_to_impersonate=None):
self.username = username
self.password = password
self.site = site
self.user_id_to_impersonate = user_id_to_impersonate

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#80, or mute the
thread
https://github.com/notifications/unsubscribe-auth/AEKwZVKly-3MvU2EfR8snDTKOk1FyMUYks5q4PnagaJpZM4Khx53
.

LGraber · 2016-10-27T21:06:18Z

taking a look now

LGraber · 2016-10-27T21:08:49Z

+    def __init__(self, name, password, embed=True):
+        self.name = name
+        self.password = password
+        self.embed = embed


Please make this an explicit property and add the property_is_boolean decorator

LGraber · 2016-10-27T21:13:22Z

+            if connection_credentials.embed:
+                credentials_element.attrib['embed'] = 'true'
+            else:
+                credentials_element.attrib['embed'] = 'false'


I believe credentials_"element.attrib['embed'] = str(connection_credentials).lower()" is all you need and you can ditch the if/else. Especially after adding the check that embed is a boolean in the credentials object

I asked him to not do that because I don't like relying on the str value, what if someone passes in an overridden True?

This makes it explicit what two values are allowed from reading the serializer

(And, minor edge case, but in PY2, True/False can be reassigned, you could make True == '3')

LGraber · 2016-10-27T21:14:35Z

Made two small requests:

Type safety on the 'embed' member
With that you can simplify your serialization code.

LGraber · 2016-10-27T21:27:35Z

what is an "overriden true"? If you don't want that, then I would prefer:
element.attrib['embed'] = 'true' if connection_credentials.embed else 'false'

I don't like 4 lines of code cause it is too bulky.

graysonarts · 2016-10-27T23:24:07Z

Under python3, you need to use relative imports

from .property_decorators import [things to import]

graysonarts

🚀

t8y8

🚀

LGraber · 2016-10-27T23:48:15Z

🚀

* Updating link in readme We renamed Examples to samples, but forgot to update the readme. * Adding tableau#80 to changelog

Adding contributing doc that was missing from the repo (tableau#74)

f140f8a

Merge branch 'development' of https://github.com/geordielad/server-cl…

6544117

…ient-python into development

geordielad closed this Oct 27, 2016

geordielad reopened this Oct 27, 2016

Add Connection Credentials

b6121d1

Allow users to add connection credentials to Datasources and workbook publish requests. Should work for smaller requests and chunked requests

geordielad force-pushed the Add-Connection-Credentials branch from fcefacc to b6121d1 Compare October 27, 2016 18:07

graysonarts approved these changes Oct 27, 2016

View reviewed changes

t8y8 suggested changes Oct 27, 2016

View reviewed changes

Making changes from PR Review

c2f697b

Explicit convert of boolean to true/false string Ordering of class properties to match signature Docstring on connection credentials and auth classes

t8y8 reviewed Oct 27, 2016

View reviewed changes

geordielad added 2 commits October 27, 2016 16:11

Fixed indenting

011a73a

reverted tableau_auth.py

be85635

geordielad added 2 commits October 27, 2016 16:34

attempting to fix codestyle errors

8d13cf4

fix indentrs and docstring

5f25ef9

t8y8 approved these changes Oct 27, 2016

View reviewed changes

t8y8 reviewed Oct 27, 2016

View reviewed changes

fix spelling error

82e84a3

LGraber reviewed Oct 27, 2016

View reviewed changes

Adding type safety on boolean embed property

c4d6cee

make import python3 friendly and true/false test more pythonic

ab478dc

graysonarts approved these changes Oct 27, 2016

View reviewed changes

t8y8 approved these changes Oct 27, 2016

View reviewed changes

graysonarts merged commit 7c41a0a into tableau:development Oct 27, 2016

graysonarts mentioned this pull request Oct 28, 2016

How to Add Connection Credentials to Publish Workbook #63

Closed

bryceglarsen pushed a commit to bryceglarsen/server-client-python that referenced this pull request Dec 28, 2023

Fix encoding bugs in Python2 (non-ASCII characters) (tableau#80)

b80333d

bryceglarsen pushed a commit to bryceglarsen/server-client-python that referenced this pull request Dec 28, 2023

Add a test to verify tableau#80 (tableau#83)

01f232f

bryceglarsen pushed a commit to bryceglarsen/server-client-python that referenced this pull request Dec 28, 2023

Add tableau#80 to changelog (tableau#92)

4ce9cfa

* Updating link in readme We renamed Examples to samples, but forgot to update the readme. * Adding tableau#80 to changelog

bryceglarsen pushed a commit to bryceglarsen/server-client-python that referenced this pull request Dec 28, 2023

Fix encoding bugs in Python2 (non-ASCII characters) (tableau#80)

ee380ac

bryceglarsen pushed a commit to bryceglarsen/server-client-python that referenced this pull request Dec 28, 2023

Add a test to verify tableau#80 (tableau#83)

86bb63b

bryceglarsen pushed a commit to bryceglarsen/server-client-python that referenced this pull request Dec 28, 2023

Add tableau#80 to changelog (tableau#92)

713eb52

* Updating link in readme We renamed Examples to samples, but forgot to update the readme. * Adding tableau#80 to changelog

Conversation

geordielad commented Oct 26, 2016

Uh oh!

graysonarts commented Oct 27, 2016

Uh oh!

geordielad commented Oct 27, 2016

Uh oh!

geordielad commented Oct 27, 2016

Uh oh!

graysonarts commented Oct 27, 2016

Uh oh!

geordielad commented Oct 27, 2016

Uh oh!

geordielad commented Oct 27, 2016

Uh oh!

graysonarts commented Oct 27, 2016

Uh oh!

graysonarts left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t8y8 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geordielad Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t8y8 commented Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t8y8 commented Oct 27, 2016

Uh oh!

t8y8 commented Oct 27, 2016

@geordielad commented on this pull request.

Uh oh!

LGraber commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t8y8 Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LGraber commented Oct 27, 2016

Uh oh!

LGraber commented Oct 27, 2016

Uh oh!

graysonarts commented Oct 27, 2016

Uh oh!

graysonarts left a comment

Choose a reason for hiding this comment

Uh oh!

t8y8 left a comment

Choose a reason for hiding this comment

Uh oh!

LGraber commented Oct 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

t8y8 left a comment •

edited

Loading

geordielad Oct 27, 2016 •

edited

Loading

t8y8 commented Oct 27, 2016 •

edited

Loading

t8y8 Oct 27, 2016 •

edited

Loading