-
Notifications
You must be signed in to change notification settings - Fork 2k
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
companion: fix uploader protocol validation #2197
Conversation
.set('uppy-auth-token', token) | ||
.set('Content-Type', 'application/json') | ||
.send({ | ||
endpoint: 'http://master.tus.com/files', |
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.
Is that endpoint correct?
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.
it doesn't matter much in this case, it's only required to be a valid URL.
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.
Then maybe we should change it to something random, so that someone doesn't get confused accidentally?
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 have updated the URL to the correct tus URL
Won't be able to review properly to get it in the next release, please feel free to merge if it works locally for you :) |
@@ -164,7 +184,7 @@ describe('download provdier file', () => { | |||
.set('uppy-auth-token', token) | |||
.set('Content-Type', 'application/json') | |||
.send({ | |||
endpoint: 'http://master.tus.com/files', | |||
endpoint: 'http://master.tus.io/files', |
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.
Wont it be confusing to see a tus endpoint while this is about s3 multipart? Even if it doesn’t matter for the test, folks will see this and draw bad conclusions or walk away in a confused state
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.
Yeah, I basically seconded that and meant to say “let’s use a random url in that case”, should have phrased it better.
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.
fixed on master
* companion: fix uploader protocol validation fixes transloadit#2188 * test: use correct tus URL * companion: make it support node 6
fixes #2188