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
Add connection credentials #80
Add connection credentials #80
Conversation
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:
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 Let me know if you have any questions. |
I think I have fixed the conflict. Do I need to do a new pull request now? |
Commenting seemed to closed the pull request Lets try that again. |
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
fcefacc
to
b6121d1
Compare
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 |
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)
Rad! Looks like it can merge correctly! Thanks! I'll take a look right now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Straight forward implementation. We'll need to add some tests before release next week, but should be easy enough to tackle. Thanks! 🚀
xml_request = ET.Element('tsRequest') | ||
datasource_element = ET.SubElement(xml_request, 'datasource') | ||
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not a shipblocker)
Since this is common code, we might consider pulling this into its own helper method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'
@@ -0,0 +1,5 @@ | |||
class ConnectionCredentials(object): | |||
def __init__(self, name, password, embed=True): | |||
self.password = password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Reorder these to match the order in the function signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -0,0 +1,5 @@ | |||
class ConnectionCredentials(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
def __init__(self, name, password, embed=True): | ||
self.name = name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indenting got all messed up here -- maybe it's tabs instead of 4 spaces?
Almost there @geordielad
Looks like your editor is using tabs instead of 4 spaces -- if using Sublime you can fix that in |
"""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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possible
🚀 |
Nah, it's ok to limit your PR to just your code for now. This is more On Oct 27, 2016 12:22, "geordielad" notifications@github.com wrote:
|
taking a look now |
def __init__(self, name, password, embed=True): | ||
self.name = name | ||
self.password = password | ||
self.embed = embed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this an explicit property and add the property_is_boolean decorator
if connection_credentials.embed: | ||
credentials_element.attrib['embed'] = 'true' | ||
else: | ||
credentials_element.attrib['embed'] = 'false' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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')
Made two small requests:
|
what is an "overriden true"? If you don't want that, then I would prefer: I don't like 4 lines of code cause it is too bulky. |
Under python3, you need to use relative imports from .property_decorators import [things to import] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
🚀 |
* Updating link in readme We renamed Examples to samples, but forgot to update the readme. * Adding tableau#80 to changelog
* Updating link in readme We renamed Examples to samples, but forgot to update the readme. * Adding tableau#80 to changelog
No description provided.