scitran / core

RESTful API
https://scitran.github.io
MIT License
18 stars 18 forks source link

Replace RAML with Swagger and removed ABAO tests #1043

Closed ehlertjd closed 6 years ago

ehlertjd commented 6 years ago

Please see the Swagger README and STYLE_GUIDE for the basics of running and writing Swagger documentation.

You can also see a list of undocumented APIs by first running the integration tests, then running (from the swagger folder):

> grunt coverage

This is ready for review, but not quite ready to merge until we resolve where the generated API Docs for master are going to live.

Review Checklist

codecov-io commented 6 years ago

Codecov Report

Merging #1043 into master will decrease coverage by 0.48%. The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1043      +/-   ##
=========================================
- Coverage   91.18%   90.7%   -0.49%     
=========================================
  Files          50      50              
  Lines        6922    6925       +3     
=========================================
- Hits         6312    6281      -31     
- Misses        610     644      +34
ehlertjd commented 6 years ago

@ryansanford Swagger 2.0 does not support specification for multiple file upload, so I will need to add an extension for code-generation to support those methods in the generated code. I will update the affected endpoints once an approach has been determined.

See swagger-api/swagger-editor#467

ehlertjd commented 6 years ago

@nagem @ambrussimon This has been rebased on master and is ready for review, thank you!

ehlertjd commented 6 years ago

@ryansanford @gsfr The devops changes are ready for review.

I moved the github setup code into .travis.yml and pass any relevant variables into bin/push-docs.sh which does the push/prune of gh-pages. This script can now be run independently of CI.

ryansanford commented 6 years ago

Looks like the tag build worked as expected. https://scitran.github.io/core/tags/swagger-pr-tag-test/

ehlertjd commented 6 years ago

@ryansanford Pretty confident about the master branch code path as well as the conflict resolution for branches/tags. I hand-tested a bunch of scenarios using a local upstream while developing the script.

I had thought about the branch documentation - do we maybe want to generate an index page for the branches/ tags/ subdirectories since github hosting doesn't seem to do that for us? If we did that, I could add those as sub-links under the main Documentation link.

ryansanford commented 6 years ago

I don't want to keep blocking this PR with scope creep, like generation and maintenance of an index page. I'd be happy with just a note in the README.md

ehlertjd commented 6 years ago

@ryansanford Note added to README.md

nagem commented 6 years ago

I think all the feedback I've had has been given verbally over the course of this project - at this point, LGTM! I've passed over the new Swagger docs to look for any abnormalities a couple of times but haven't spotted any. Doesn't mean we're clear of them, but I'd also like to go through and up the quality of some of the original docs. I'm sure any inconsistencies we missed can be found then. If it passes tests and passes a best-effort visual review, I'm good to go.

nagem commented 6 years ago

I still have to commit the docs I wrote for some report endpoints. Will mark my review as complete but wanted to note those are coming soon.