Expose the fileuploads API endpoint
#894
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@bcantoni it looks like the action changes broke all the CI tests? It fails on the slack message and then stops. I wouldn't recommend merging this until the tests are running again |
t8y8
previously approved these changes
Sep 20, 2021
There was a problem hiding this comment.
🚀 I don't know why we really need it exposed but I agree it makes more sense, is more consistent, and is flexible enough for the future.
I'm a little worried about how deep it goes though -- when the pipeline is fixed we want to verify the tests pass, and probably run a manual test and post the results.
|
@t8y8 I'll take a look - we had some Slack workspace changes over the weekend and it's possible that's what broke. (Maybe the webhook URL has moved.) @vogelsgesang Could you please rebase these PRs off the |
vogelsgesang
force-pushed
the
expose-fileuploads
branch
from
d1a4cbc
to
5e5b272
Compare
|
@bcantoni I rebased on development branch and updated the pull request accordingly @t8y8 we don't necessarily need to expose it. If you prefer, we could also mount it under the |
We had at least two independent re-implementations [1, 2] of file uploads within the last 4 months. And this was despite the fact that both projects already used TSC which would offer this functionality. Currently, the upload functionality in TSC is hard to discover as it is not exposed like all other REST functions. Instead of `server.fileuploads`, one has to first create an instance of the (undocumented) `Fileuploads` class. The upload functionality was probably because it should be usually unnecessary: The uploaded files are usually part of publishing a workbook/datasource/... and the corresponding `datasources.publish` (and similar) already take care of the upload internally. However, TSC isn't always up-to-date with new REST APIs, and by exposing file uploads directly we can make sure to offer the best possible experience to users of TSC also in those transition periods. This commit: * turns the `Fileuploads` class into a normal endpoint class which is not tied to one upload (So far, `Fileuploads` was not stateless. Now it is) * adds the endpoint to `server`, such that file uploads are available as `server.fileuploads` * adjusts all other users to use `server.fileuploads` instead of constructing an ad hoc instance of the `Fileuploads` class Documentation will be added in a separate commit. [1] https://github.com/jharris126/tableau-data-update-api-samples/blob/41f51ae4d220de55caf63e91fe9eff5694b9456a/basic/basic_incremental_load.py#L23 [2] https://github.com/tableau/hyper-api-samples/blob/382e66481ec8339407cf9cfa5d41fcdcf3f6a0fb/Community-Supported/clouddb-extractor/tableau_restapi_helpers.py#L165
vogelsgesang
force-pushed
the
expose-fileuploads
branch
from
5e5b272
to
95bb0ca
Compare
vogelsgesang
force-pushed
the
expose-fileuploads
branch
from
9266786
to
3abe4e9
Compare
|
For some reason, Github Actions on this repo still don't trigger for me. Despite adding additional commits/force-pushes... There was a successful test run in https://github.com/vogelsgesang/server-client-python/actions/runs/1265585205, though.
I patched
The upload succeeded with the log showing that we indeed used a separate file upload: @t8y8 @bcantoni are we fine with merging this (also take into account the additional commit regarding exception safety which I added to this PR) |
|
Yeah, I'm fine with merging given the manual test runs (I think if you branch from master, even if you rebase the PR on development, that prevents the CI from running. Don't know why) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We had at least two independent re-implementations [1, 2] of file uploads
within the last 4 months. And this was despite the fact that both projects
already used TSC which would offer this functionality.
Currently, the upload functionality in TSC is hard to discover as it is not
exposed like all other REST functions. Instead of
server.fileuploads, onehas to first create an instance of the (undocumented)
Fileuploadsclass.The upload functionality was probably because it should be usually unnecessary:
The uploaded files are usually part of publishing a workbook/datasource/...
and the corresponding
datasources.publish(and similar) already take care ofthe upload internally.
However, TSC isn't always up-to-date with new REST APIs, and by exposing file
uploads directly we can make sure to offer the best possible experience to
users of TSC also in those transition periods.
This commit:
Fileuploadsclass into a normal endpoint class which is not tiedto one upload (So far,
Fileuploadswas not stateless. Now it is)server, such that file uploads are available asserver.fileuploadsserver.fileuploadsinstead of constructingan ad hoc instance of the
FileuploadsclassDocumentation will be added in a separate commit.
[1] https://github.com/jharris126/tableau-data-update-api-samples/blob/41f51ae4d220de55caf63e91fe9eff5694b9456a/basic/basic_incremental_load.py#L23
[2] https://github.com/tableau/hyper-api-samples/blob/382e66481ec8339407cf9cfa5d41fcdcf3f6a0fb/Community-Supported/clouddb-extractor/tableau_restapi_helpers.py#L165