Skip to content
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

Initial OpenAPI specification of the tus protocol. #170

Merged
merged 10 commits into from
Jan 28, 2021

Conversation

RubenGarcia
Copy link
Contributor

Following #169

@RubenGarcia
Copy link
Contributor Author

Is 460 Checksums mismatch used somewhere else outside of tus?

@RubenGarcia RubenGarcia marked this pull request as draft October 5, 2020 14:50
@RubenGarcia
Copy link
Contributor Author

This part is missing:

The Tus-Resumable header MUST be included in every request and response except for OPTIONS requests. The value MUST be the version of the protocol used by the Client or the Server.

If the version specified by the Client is not supported by the Server, it MUST respond with the 412 Precondition Failed status and MUST include the Tus-Version header into the response. In addition, the Server MUST NOT process the request.

@Acconut
Copy link
Member

Acconut commented Oct 10, 2020

Thank you for your incredible work here! Could you help me, as person who hasn't used OpenAPI before, with some basic questions:

  1. What is the difference between OpenAPI 2 and 3? Do we need both version or could we just use the newer one?
  2. How can people use these files? What tools does it integrate with?
  3. I see redundant definition of some header (e.g. Tus-Resumable and Upload-Offset). Would it be possible to deduplicate them?

@RubenGarcia
Copy link
Contributor Author

RubenGarcia commented Oct 12, 2020

  1. OpenAPI 3 is the newer, more capable standard. But many tools still use OpenAPI 2, so it's worth keeping both.
    There are tools and websites to convert and help in creating the files, such as swagger.io.
  2. Once you have those files, there are tools to create http servers and clients to access the API; missing functionality (semantics) is then to be added manually to the relevant code snippets. e.g. go-swagger, or the generators available at swagger.io.
  3. Yes, in OpenAPI 3. No in OpenAPI 2.0 https://swagger.io/docs/specification/2-0/describing-responses/. Once the OpenAPI 2.0 is complete and is in a state where merge is possible, I will convert to 3.0 and optimize.

@RubenGarcia
Copy link
Contributor Author

Do you see anything I may have missed from the standard in the OpenAPI 2.0 file?

@Acconut
Copy link
Member

Acconut commented Oct 12, 2020

There are tools and websites to convert

Would it then make sense to only keep the OpenAPI 3 version in this repository and let the users convert it to OpenAPI 2 when necessary? I am not really a fan of having two OpenAPI files with basically the same content in this repository. Maintaining two files which are similar but also have differences is quite erroneous, especially since most contributors here are not OpenAPI experts (including me). That would also solve the problem with the duplicate header definitions.

@smatsson
Copy link

@RubenGarcia Your work here is truly amazing. Nice done!

@RubenGarcia
Copy link
Contributor Author

I just checked https://editor.swagger.io. You can convert from 2.0 to 3.0 but not from 3.0 to 2.0, I think because 3.0 added more functionality.
And since the 3.0 would needs to be modified to merge the duplicate definitions, I think there is value in keeping both.
I don't think the effort of keeping both is too high, because the protocol is quite stable now.

@Acconut
Copy link
Member

Acconut commented Oct 14, 2020

From a quick Google search, I found https://lucybot-inc.github.io/api-spec-converter/ and https://github.com/LucyBot-Inc/api-spec-converter for converting from 3.0 to 2.0. Of course, I didn't try them yet. Do you have experience with them and can judge if they are helpful or not?

because the protocol is quite stable now.

Yes, the protocol is stable but we have some new extensions lined up. So there are likely going to be some additions in the next weeks.

@RubenGarcia
Copy link
Contributor Author

I was not aware of these. Ok, then I'll make a 3.01 and add a note to use these as needed if you need 2.0.

@RubenGarcia RubenGarcia marked this pull request as ready for review October 15, 2020 07:36
@RubenGarcia
Copy link
Contributor Author

I moved the definitions of the headers which are used in various places to the end to avoid repetition.
I kept the two separate instances of Content-Length because they have different semantics (i.e. one of them must be 0 if no further extensions are available).
I kept only OpenAPI 3.0 and added a comment in the readme about conversion software.

@Acconut
Copy link
Member

Acconut commented Oct 15, 2020

Thank you very much, Ruben! I will have a more in depth look at the content next week. Do you know if a good validator/linter exists, so we can ensure that we don't commit invalid OpenAPI specifications?

@RubenGarcia
Copy link
Contributor Author

I am using the swagger editor on swagger.io

@Acconut
Copy link
Member

Acconut commented Oct 29, 2020

I am using the swagger editor on swagger.io

Ok, I was more thinking about a CLI tool or similar that we could automatically run for every commit to ensure that the OpenAPI file is not malformed. Do you know if that exists?

@RubenGarcia
Copy link
Contributor Author

RubenGarcia commented Nov 2, 2020

Other tools:

Or this one

Would you like me to setup super-linter in this repository?

@Acconut
Copy link
Member

Acconut commented Jan 18, 2021

Hello @RubenGarcia, I want to apologize for my late reply! I do not know how but I lost this discussion totally from my radar! I am deeply sorry for that after all of the efforts you invested into this!

super-linter in combination with Spectral looks like the tools we had in mind! Are you still interested in moving forward with this topic?

@RubenGarcia
Copy link
Contributor Author

RubenGarcia commented Jan 19, 2021

Yes. I don't have experience with superlinter, but I think it's worth exploring. Do I need administrator priviledges or anything special?

@RubenGarcia
Copy link
Contributor Author

I added the configuration using
https://github.com/github/super-linter
and
https://github.com/marketplace/actions/spectral-linting
but I think CICD needs to be setup on your side before it will run.

@Acconut
Copy link
Member

Acconut commented Jan 28, 2021

Thank you very, very much! I will merge this, so we can see if the GH Action works. We can iterate over the specification in further PRs.

@Acconut Acconut merged commit f4c98d1 into tus:master Jan 28, 2021
@Acconut
Copy link
Member

Acconut commented Jan 28, 2021

Seems like there is still a configuration problem with bad GitHub credentials: https://github.com/tus/tus-resumable-upload-protocol/runs/1782659818?check_suite_focus=true#step:4:8

@RubenGarcia
Copy link
Contributor Author

Seems like there is still a configuration problem with bad GitHub credentials: https://github.com/tus/tus-resumable-upload-protocol/runs/1782659818?check_suite_focus=true#step:4:8

I just checked that link, and it shows green (success) for me, so I guess all is ok now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants