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

remove ICS categories from swagger #4429

Merged
merged 2 commits into from
Jun 20, 2019
Merged

remove ICS categories from swagger #4429

merged 2 commits into from
Jun 20, 2019

Conversation

hschoenburg
Copy link
Contributor

  • 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)

@fedekunze fedekunze changed the title remove ICS categories from swagger UI docs Fixes #4357 remove ICS categories from swagger UI docs May 29, 2019
@fedekunze fedekunze changed the title remove ICS categories from swagger UI docs remove ICS categories from swagger May 29, 2019
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

@alexanderbez
Copy link
Contributor

@hschoenburg we now commit the generated statik file. Please run: make update-swagger-docs and commit the resulting file :)

@alexanderbez alexanderbez added T:Docs Changes and features related to documentation. ready-for-review labels May 29, 2019
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 -- needs swagger docs build updated.

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.

Thanks Hans ! Few things missing:

  • /auth/accounts/{address} is still under ICS1 tag, should have tag Auth
  • Missing Mint tag for mint requests
  • GET /node_info is duplicated, need to delete the one with Misc tag

@hschoenburg
Copy link
Contributor Author

Thanks for the comments guys. I added a newly generated statik.go, as well as Auth and Mint tags. @fedekunze I wasn't completely clear on which node_* endpoint was duplicate but I think I removed the right one (with the Misc tag).

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.

ACK

@fedekunze
Copy link
Collaborator

check_statik test failing

@hschoenburg
Copy link
Contributor Author

sorry forgot to fix the statik.go check until now. Lmk if there is anything more I can do here. Looks like the PR needs to be merged locally because of the statik.go conflict.

@alexanderbez
Copy link
Contributor

There is a conflict @hschoenburg. Perhaps the statik file you generated should be the one committed. How did you generate it?

@hschoenburg
Copy link
Contributor Author

There is a conflict @hschoenburg. Perhaps the statik file you generated should be the one committed. How did you generate it?

@alexanderbez >> thanks for following up. Since this PR makes changes to the swagger.yaml I imagine the statik.go in this branch is the one that should be merged into master. After committing the changes to swagger.yaml I ran $ make update-swagger-docs and committed the new statik.go file. Was that the right process? The statik check it passing now. But because of the conflict I think this merge needs to be done locally?

@alexanderbez
Copy link
Contributor

That is the correct process @hschoenburg -- simply commit your statik.go file in favor of the one on master to resolve the conflicts :)

@sabau
Copy link
Contributor

sabau commented Jun 4, 2019

Shouldn't we remove also the implementation along with the documentation? Or we end up having those two working but not documented:

  • /staking/delegators/{delegatorAddr}/txs

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.

need to resolve conflicts and add clog entry

@fedekunze fedekunze mentioned this pull request Jun 7, 2019
5 tasks
@fedekunze
Copy link
Collaborator

@hschoenburg can we have this for the release next week?

@codecov
Copy link

codecov bot commented Jun 20, 2019

Codecov Report

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

@@           Coverage Diff           @@
##           master    #4429   +/-   ##
=======================================
  Coverage   52.75%   52.75%           
=======================================
  Files         264      264           
  Lines       16464    16464           
=======================================
  Hits         8685     8685           
  Misses       7130     7130           
  Partials      649      649

@tac0turtle
Copy link
Member

tac0turtle commented Jun 20, 2019

need to resolve conflicts and add clog entry

decided to not add a clog entry, spoke with team in berlin

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 -- no pending log entry needed imho.

@tac0turtle
Copy link
Member

also, the commit message steemed from something else I apologize for it

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.

utACK

@alessio alessio merged commit c3bef9a into master Jun 20, 2019
@alessio alessio deleted the hans/swagger-ics-update branch June 20, 2019 12:18
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.

6 participants