Suggestion for Workbook Update Connection #149
Conversation
| def update_req(self, connection_item): | ||
| xml_request = ET.Element('tsRequest') | ||
| connection_element = ET.SubElement(xml_request, 'connection') | ||
| connection_element.attrib['serverAddress'] = connection_item.server_address.lower() |
There was a problem hiding this comment.
May as well add the other attributes, any reason you don't want to?
(At least port and password would cover some folks use cases)
There was a problem hiding this comment.
No particular reason - I was just editing in parts that I needed to change. Probably a good idea to add the other ones too.
docs/docs/api-ref.md
Outdated
| Updates a workbook connection string. The workbook connections must be populated before they strings can be updated. | ||
|
|
||
| ```py | ||
| Workbooks.update_conn(workbook, workbook.connections[i] |
|
|
||
| ```py | ||
| server.workbooks.populate_connections(workbook) | ||
| workbook.connections[i].server_address = 'new_endpoint' |
There was a problem hiding this comment.
This is technically correct, but I don't like the pattern (Not your fault, the way we try to keep the models and the upstream in sync are a little wonky)
Can you document it like this:
server.workbooks.populate_connections(workbook)
conn_to_update = workbook.connections[0] # Or whichever you want, but i is context-less and can't be copy/pasted
conn_to_update.server_address = 'abcdefg'
conn_to_update.port = 1337
server.workbooks.update_conn(workbook, conn_to_update)/cc @RussTheAerialist
There was a problem hiding this comment.
That works - I'll make those changes.
|
Changes made to reflect @t8y8 comments |
There was a problem hiding this comment.
Looks like pycodestyle is failing on these items:
1.53s$ pycodestyle tableauserverclient test
tableauserverclient/server/request_factory.py:18:1: E302 expected 2 blank lines, found 1
tableauserverclient/server/request_factory.py:315:1: E302 expected 2 blank lines, found 1
tableauserverclient/server/request_factory.py:331:1: E302 expected 2 blank lines, found 1
tableauserverclient/server/endpoint/workbooks_endpoint.py:96:1: W293 blank line contains whitespace
tableauserverclient/server/endpoint/workbooks_endpoint.py:98:99: W291 trailing whitespace
And then a copy/paste error and it looks good after that!
| if connection_item.server_address: | ||
| connection_element.attrib['serverAddress'] = connection_item.server_address.lower() | ||
| if connection_item.server_port: | ||
| connection_element.attrib['port'] = connection_item.server_address |
There was a problem hiding this comment.
copy/paste error :)
And may as well make it str(connection_item.server_port) so folks can use a string or an int
| conn_to_update.server_port = 1234 | ||
| conn_to_update.username = 'username' | ||
| conn_to_update.password = 'password' | ||
| conn_to_update.embed_password = TRUE/FALSE |
There was a problem hiding this comment.
Oh, this should just be True or False in Python, the all caps could be a different variable
| @@ -312,7 +313,8 @@ def publish_req_chunked(self, workbook_item, connection_credentials=None): | |||
| parts = {'request_payload': ('', xml_request, 'text/xml')} | |||
| return _add_multipart(parts) | |||
|
|
|||
| class WorkbookConnection(object): | |||
|
|
|||
| class WorkbookConnection(object): | |||
There was a problem hiding this comment.
Oops, I don't think this should be indented
|
Current fixes complete |
|
Looks good to me. At some point we might rethink how our "update" works. It is supposed to figure out what needs to be updated (based on anything that has changed) but it has gotten lax. Thus you need updateConn. If not, you might just call "update". |
|
Thanks!! |
|
I edited your commit messages and merged. Congrats on your first accepted PR, @cmtoomey !! |
|
Yay - thanks @t8y8...now just to wait until it ships |
Suggestion for Workbook Update Connection (tableau#149)
* adding oracle param support related to tableau#137 * fix copy paste error
…)" (tableau#150) This reverts commit 5d1a81e.
Per #148, an option for updating workbook connections. Having to iterate over each connection individually is sub-optimal, but core REST API only allows one connection update at a time.
This only accounts for connection string updates and not ports, username, or password changes.
Use case is DB migration.
Also - I'm not sure why the two HTML files were included. No edits were made after the fork.