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

Header information #83

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

WouterJanson
Copy link
Contributor

  • Adds the application/vnd.signhost.v1+json Accept header to the curl examples.
  • Adds a section with default headers to the endpoint page.

See #82

endpoints.html Outdated
<th>Description</th>
</thead>
<tbody>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this newline?

required: true
ApiKeyParam:
value: |
`APPKey {Appkey}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

key => Key

endpoints.html Outdated
@@ -9,6 +9,27 @@
<p>All input strings should be UTF-8 encoded, our response is always UTF-8 encoded.</p>
<p>Some of our endpoints return both the status of the transaction and signer activities. Further details about the statuses and activities can be found <a href="/status-activity/">here</a>.</p>

<h3>Default Headers</h3>
<p>In order to communicate with Signhost, the API is expecting the following http headers for all requests. The parts between curlybraces indicate values that need to be replaced.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would result in saying we require the json version header in every request. But not all requests return JSON, so I don't think this is correct

Comment on lines 13 to 14
parameters:
AppkeyParam:
headers:
AppKey:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this new in swagger? Or did you break swagger compatability?

@WouterJanson
Copy link
Contributor Author

Updated:

  • Headers are added to the swagger file based on the swagger 2.0 spec.

@WouterJanson WouterJanson force-pushed the HeaderInformation branch 2 times, most recently from 16cf4b2 to ab2c5a0 Compare October 13, 2020 11:22
in: header
type: string
required: true
ApiKeyParam:
default: application/vnd.signhost.v1+json
Copy link
Collaborator

@ATimmeh33 ATimmeh33 Oct 14, 2020

Choose a reason for hiding this comment

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

I think default might be confusing, because:

  1. it MUST be explicitly to use new features like the authentication array
  2. this is only valid for JSON

Let's leave this open and see @MrJoe 's thoughts on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. this is only valid for JSON

Yes, that's why this parameter is only added to specific endpoints in the swagger file, so in that case default would make sense and it's also the way to add a value according to the spec. (Which I broke in the previous version of this PR 😅)

However I do agree that it might be confusing phrased in the documentation page itself.

{% endfor %}
</tbody>
</table>
<!-- End list of all HTTP headers -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing EOF

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

Successfully merging this pull request may close these issues.

3 participants