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

added callback specs #176

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

added callback specs #176

wants to merge 3 commits into from

Conversation

sgalpha01
Copy link

I've added callback schema according OpenAPI v3 specs. Kindly review it and suggest necessary changes, if required.

Copy link
Contributor

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

Looks good but some points need some work, see comments.

openapi/task_execution_service.openapi.yaml Outdated Show resolved Hide resolved
openapi/task_execution_service.openapi.yaml Outdated Show resolved Hide resolved
openapi/task_execution_service.openapi.yaml Outdated Show resolved Hide resolved
openapi/task_execution_service.openapi.yaml Outdated Show resolved Hide resolved
@sgalpha01 sgalpha01 marked this pull request as ready for review July 10, 2022 04:35
@kellrott
Copy link
Member

I'm not sure if this PR should be targeting develop. That is the current destination for TES 1.1. I don't think that callbacks will be part of 1.1. Maybe we should create a develop-1.2 branch to hold these more forward looking features.

openapi/task_execution_service.openapi.yaml Outdated Show resolved Hide resolved
openapi/task_execution_service.openapi.yaml Outdated Show resolved Hide resolved
openapi/task_execution_service.openapi.yaml Outdated Show resolved Hide resolved
@aniewielska
Copy link
Contributor

@kellrott - think it can target develop, but will be merged after other changes get there.

@aniewielska
Copy link
Contributor

BTW, we are quite inconsistent in the choice of camel vs snake case, but it has happened before this PR.

@uniqueg
Copy link
Contributor

uniqueg commented Sep 11, 2022

@aniewielska: While you are right about snake_case/camelCase inconsistencies in the TES specs, I don't see any new inconsistencies being introduced here. The PR follows usage in the current specs (e.g., schema names in camelCase, snake_case for schema properties). In the one case that is not already covered elsewhere in the specs (the name for the callback), it uses camelCase as does the exmaple in the OpenAPI 3.0 docs. I think this is reasonable.

@@ -146,6 +146,23 @@ paths:
application/json:
schema:
$ref: '#/components/schemas/tesCreateTaskResponse'
callbacks:
statusChange:
'{$request.body#/callback_url}':

Choose a reason for hiding this comment

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

This cool feature was mentioned at the GA4GH call, so I took a peek... I'm curious, how is authentication supposed to work for the callback url?

Choose a reason for hiding this comment

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

github allows a simple password for webhooks but does not require it. I wonder if the callback should be an object instead of a single url:

"callback": {
  "url": "your url goes here",
  "headers": [
   { "key":"value"}
  ]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea here was not to put callback url behind extra authentication (the URL itself might be a secret?) and additionally advise consumers of the callback to verify the information by hitting the GET endpoint.

Copy link

@patmagee patmagee Sep 21, 2022

Choose a reason for hiding this comment

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

@aniewielska thanks for the clarification. I wonder if it would still potentially make sense to make this an object to future proof in the case of wanting to add additional access mechanisms in the future

Choose a reason for hiding this comment

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

The idea here was not to put callback url behind extra authentication (the URL itself might be a secret?) and additionally advise consumers of the callback to verify the information by hitting the GET endpoint.

I think it would be nice to have your clarification in the spec.

I'm still slightly concerned about needing to expose a public callback url without auth, and I think Patrick's proposal addresses that, but I don't feel too strongly (and I'm not a reviewer anyway, just lurking :) ).

Copy link
Contributor

Choose a reason for hiding this comment

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

Good points here, thanks. Agree with future proofing by making it an object and extending the description to clarify that clients should verify the info from the callback and that the callback URL may include a secret

@@ -219,6 +236,19 @@ components:
- FULL

schemas:
tesCallbackStatus:

Choose a reason for hiding this comment

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

How will the the callback know which TES server is making the request? Is the callback expected to look at the Origin header (but I don't think that's guaranteed to be set)?

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

Successfully merging this pull request may close these issues.

6 participants