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.

Sign up for GitHub

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

Jump to bottom

feat: add BLOCKSCOUT_HOST, and use it in API docs #2193

Merged
merged 2 commits into from
Jun 25, 2019
Merged

feat: add BLOCKSCOUT_HOST, and use it in API docs #2193

merged 2 commits into from
Jun 25, 2019

Conversation

zachdaniel
Copy link
Contributor

@zachdaniel zachdaniel commented Jun 18, 2019 •
edited
Loading

Motivation

  • Right now the host is wrong in the API documentation page.

Enhancements

  • This uses the correct value to render the host in api docs, and allows it to be configured via BLOCKSCOUT_HOST env variable.

    • I added an entry to CHANGELOG.md with this PR
    • If I added new functionality, I added tests covering it.
    • If I fixed a bug, I added a regression test to prevent the bug from silently reappearing again.
    • I checked whether I should update the docs and did so if necessary

@coveralls
Copy link

coveralls commented Jun 18, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 94f19dc3-5da1-4ad8-8573-a89e07539112

  • 2 of 3 (66.67%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.007%) to 80.962%
Changes Missing Coverage Covered Lines Changed/Added Lines %
apps/block_scout_web/lib/block_scout_web/views/api_docs_view.ex 2 3 66.67%
Totals Coverage Status
Change from base Build 4d4c76eb-21a8-40ff-979f-4f765b9a22da: -0.007%
Covered Lines: 5001
Relevant Lines: 6177
💛 - Coveralls

ayrat555
ayrat555 reviewed Jun 19, 2019
View reviewed changes
apps/block_scout_web/lib/block_scout_web/templates/api_docs/index.html.eex Outdated
@@ -2,7 +2,7 @@
<div class="card">
<div class="card-body">
<h1 class="card-title margin-bottom-sm"><%= gettext("API Documentation") %></h2>
<p class="api-text-monospace" data-endpoint-url="<%= BlockScoutWeb.Endpoint.url() %>/api">[ <%= gettext "Base URL:" %> <%= @conn.host %>/api ]</p>
<p class="api-text-monospace" data-endpoint-url="<%= BlockScoutWeb.Endpoint.url() %>/api">[ <%= gettext "Base URL:" %> <%= BlockScoutWeb.Endpoint.url() %>/api ]</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it include a port in the URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I thought that I had confirmed that port was present in that case. I'll take a look.

Copy link
Member

Choose a reason for hiding this comment

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

I see port in the api docs page for JSON RPC, but it is missing in ETH JSON RPC page

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, port is included in that already, yes.

ayrat555
ayrat555 approved these changes Jun 19, 2019
View reviewed changes
Copy link
Contributor

@ayrat555 ayrat555 left a comment

Choose a reason for hiding this comment

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

One comment.

vbaranov added a commit that referenced this pull request Jun 19, 2019
@vbaranov
#2193
7c48edc
@vbaranov
Copy link
Member

@zachdaniel

Port is not returned in the api docs page for Eth RPC endpoints
Screenshot 2019-06-20 at 10 56 47

@zachdaniel
Copy link
Contributor Author

zachdaniel commented Jun 20, 2019 •
edited
Loading

@vbaranov if you merge the other PR for the compatibility matrix, I can include this change for that page in this PR.

@vbaranov
Copy link
Member

@vbaranov if you merge the other PR for the compatibility matrix, I can include this change for that page in this PR.

@zachdaniel #2167 has been merged

vbaranov pushed a commit that referenced this pull request Jun 21, 2019
@zachdaniel @vbaranov
#2193
64a756f
@vbaranov vbaranov merged commit 9f3ba2c into master Jun 25, 2019
@vbaranov vbaranov deleted the add_runtime_configured_host branch June 25, 2019 14:23
@ayrat555 ayrat555 mentioned this pull request Jul 1, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pasqu4le pasqu4le pasqu4le approved these changes

@ayrat555 ayrat555 ayrat555 approved these changes

@vbaranov vbaranov vbaranov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zachdaniel @coveralls @vbaranov @ayrat555 @pasqu4le