Multi-Credential Support in TSC #276
Conversation
…thon into 275-feature-multiconn
| @@ -32,11 +32,11 @@ def _safe_to_log(server_response): | |||
| '''Checks if the server_response content is not xml (eg binary image or zip) | |||
| and and replaces it with a constant | |||
| ''' | |||
| ALLOWED_CONTENT_TYPES = ('application/xml',) | |||
| ALLOWED_CONTENT_TYPES = ('application/xml', 'application/xml;charset=utf-8') | |||
There was a problem hiding this comment.
I should allow this type through as well, missed it in first PR
| if server_response.headers.get('Content-Type', None) not in ALLOWED_CONTENT_TYPES: | ||
| return '[Truncated File Contents]' | ||
| else: | ||
| return server_response.content | ||
| return server_response.content[:300] |
| @@ -231,15 +238,18 @@ def publish(self, workbook_item, file_path, mode, connection_credentials=None): | |||
| upload_session_id = Fileuploads.upload_chunks(self.parent_srv, file_path) | |||
| url = "{0}&uploadSessionId={1}".format(url, upload_session_id) | |||
| xml_request, content_type = RequestFactory.Workbook.publish_req_chunked(workbook_item, | |||
There was a problem hiding this comment.
I need to pipe this through to 'chunked' code paths as well
| if connection_credentials is not None and connections is not None: | ||
| raise RuntimeError('You cannot set both `connections` and `connection_credentials`') | ||
|
|
||
| if connection_credentials is not None: |
There was a problem hiding this comment.
This is redundant, will remove
| return ET.tostring(xml_request) | ||
|
|
||
| def _add_connections_element(self, connections_element, connection): |
There was a problem hiding this comment.
Can make these static, or move to a helper class for datasources to share
|
The rabbit hole goes deeper -- I'm adding parsers so we can extract returned values from requests... at least for testing. |
|
Alright, I'm ready to declare this ready for review. But I realized something: We don't support multi-credential Data Source publish, so you can't publish a federated data source that requires multiple credentials. Only workbooks. I can leave the code in for data sources and protect that parameter with a Or I can remove it, let me know |
t8y8
changed the title
| @@ -230,16 +237,21 @@ def publish(self, workbook_item, file_path, mode, connection_credentials=None): | |||
| logger.info('Publishing {0} to server with chunking method (workbook over 64MB)'.format(filename)) | |||
| upload_session_id = Fileuploads.upload_chunks(self.parent_srv, file_path) | |||
| url = "{0}&uploadSessionId={1}".format(url, upload_session_id) | |||
| conn_creds = connection_credentials | |||
There was a problem hiding this comment.
Is conn_creds variable necessary?
There was a problem hiding this comment.
Only because of line-length limits 🤷♂️
|
|
||
| if connection_credentials is not None: | ||
| import warnings | ||
| warnings.warn("connection_credendials is being deprecated. Use connections instead, see http://...", |
There was a problem hiding this comment.
Typo in connection_credendials. Also I'm not too familiar with connection_credentials but should this warning also be in datasources_endpoint.py?
There was a problem hiding this comment.
If this were supported in Datasources then yes, but it turns out my code is ahead of the curve here, datasources don’t do multi-credential publishing yet.
Following on from @marianotn's work, I rebased on top of the latest Development and reorganized a few things for readability and back-compat.
Still need to update tests, I'm probably going to test
RequestFactory.Workbook.publish_reqspecifically, which would be one of the few serializer tests, but I think it's worth the coverage.Thoughts on the approach? I tried to make it so connections would work older than 2.8, but it was getting really nasty.
I also need to add support to Data Sources too.