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

Update swagger file with tested GET routes #4470

Merged
merged 5 commits into from
Jun 4, 2019
Merged

Conversation

sabau
Copy link
Contributor

@sabau sabau commented Jun 3, 2019

A route had to be commented as it's returning 500 instead of 200 with empty body -> will make the tests fail:

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: clog add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Signed-off-by: Karoly Albert Szabo <szabo.karoly.a@gmail.com>
Signed-off-by: Karoly Albert Szabo <szabo.karoly.a@gmail.com>
@sabau sabau mentioned this pull request Jun 3, 2019
5 tasks
@sabau sabau added ready-for-review T:Docs Changes and features related to documentation. labels Jun 3, 2019
@codecov
Copy link

codecov bot commented Jun 3, 2019

Codecov Report

Merging #4470 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #4470   +/-   ##
======================================
  Coverage    54.6%   54.6%           
======================================
  Files         250     250           
  Lines       16076   16076           
======================================
  Hits         8779    8779           
  Misses       6650    6650           
  Partials      647     647

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pending changelog entry and TODO comment on the slashing info

client/lcd/swagger-ui/swagger.yaml Show resolved Hide resolved
sabau added 2 commits June 3, 2019 15:26
Signed-off-by: Karoly Albert Szabo <szabo.karoly.a@gmail.com>
Signed-off-by: Karoly Albert Szabo <szabo.karoly.a@gmail.com>
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- one minor nit :)

@@ -234,11 +234,17 @@ paths:
- application/json
parameters:
- in: query
name: tag
name: action
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the name action here? It can be any key. Can we omit name?

Copy link
Contributor Author

@sabau sabau Jun 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this situation, it's to have a working example, I wanted to add a link to the tags page in the docs to specify you could add any tag defined there, but it won't work until we update from swagger 2.0 to OpenAPI 3.0

@@ -0,0 +1 @@
align swagger file to run contract tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this pending log entry...not very informative imho. Perhaps we either remove it or state what's actually of tangible use to the consumer?

Copy link
Contributor Author

@sabau sabau Jun 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@alessio alessio merged commit 3962b3c into master Jun 4, 2019
@alessio alessio deleted the sabau/update-swagger-file branch June 4, 2019 17:20
alessio pushed a commit to cosmos/gaia that referenced this pull request Jun 8, 2019
bchainexpt pushed a commit to bchainexpt/Blockchain-wallet-andorid-app that referenced this pull request Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants