Add pingback support (bug 1377305) #16
Conversation
zalun
force-pushed
the
add-callback
branch
2 times, most recently
from
924f1a5 to
568b1a8
Compare
landoapi/models/landing.py
Outdated
| @@ -42,35 +51,45 @@ class Landing(db.Model): | |||
| request_id = db.Column(db.Integer, unique=True) | |||
| revision_id = db.Column(db.String(30)) | |||
| status = db.Column(db.Integer) | |||
| auth = db.Column(db.String(30)) | |||
There was a problem hiding this comment.
I don't think the auth mechanism should be stored in the database. The security design doc says we should consider a shared API key for validating the client doing the update, and that means we'll end up adding a second migration at a later to remove the auth column. I recommend we split this user story into two parts: one for this patch to perform status updates with no auth, and a second story to have a special API key for a pingback client to use to update statuses.
zalun
force-pushed
the
add-callback
branch
2 times, most recently
from
f108205 to
4f3ae83
Compare
docker-compose.yml
Outdated
| @@ -17,6 +17,7 @@ services: | |||
| - PHABRICATOR_UNPRIVILEGED_API_KEY=api-123456789 | |||
| - TRANSPLANT_URL=https://stub.transplant.example.com | |||
| - DATABASE_URL=sqlite:////db/sqlite.db | |||
| - HOST_URL=https://landoapi.mozilla.org | |||
There was a problem hiding this comment.
Agree that we will need this variable. Lets set to http://lando-api.test and in ckolos change it for dev, stage, and prod.
| description: | | ||
| Retrieve status of the landing job | ||
| required: true | ||
| schema: |
There was a problem hiding this comment.
See https://docs.google.com/document/d/1cRIdpVqYcn0Mle7vyjJePNw5TcFoZ20N42dLVy5xVEk/edit# for the remaining data.
landoapi/api/landings.py
Outdated
| ) | ||
| landing.status = data['status'] | ||
| landing.save() | ||
| return landing.serialize(), 202 |
There was a problem hiding this comment.
Does transplant expect a 202, or 200?
https://httpstatuses.com/202 vs https://httpstatuses.com/200
There was a problem hiding this comment.
I think it just checks for errors
landoapi/models/landing.py
Outdated
| from landoapi.models.storage import db | ||
| from landoapi.phabricator_client import PhabricatorClient | ||
| from landoapi.transplant_client import TransplantClient | ||
|
|
||
| TRANSPLANT_JOB_STARTING = 'starting' |
There was a problem hiding this comment.
Just for me, what is the difference between 'starting' and 'started'?
There was a problem hiding this comment.
It is set before request is made. No idea if it is started on the Transplant side.
There was a problem hiding this comment.
It is set before request is made. No idea if it is started on the Transplant side.
'pending' might be a better word to communicate the status.
zalun
force-pushed
the
add-callback
branch
5 times, most recently
from
9a8f50d to
45fd5ce
Compare
|
|
||
| landing.error = data.get('error_msg', '') | ||
| landing.result = data.get('result', '') | ||
| landing.status = TRANSPLANT_JOB_LANDED if data['landed' |
There was a problem hiding this comment.
This line confuses me. If the 'landed' field is present, regardless of value, everything is OK, and if an update is ever made with this field missing, it's a failure in the other system? I think there should be a comment or two in here to explain what the possible combinations of incoming values are, and to explain what they represent in the other system and in our system.
There was a problem hiding this comment.
I'm adding description of fields which are received from Transplant (from Google doc)
| trans = TransplantClient() | ||
| callback = '%s/landings/%s/update' % ( | ||
| os.getenv('HOST_URL'), landing.id |
There was a problem hiding this comment.
This setting's name could be improved. I can't tell from the name alone that it is used for pingbacks. Maybe something like PINGBACK_HOST_URL or just PINGBACK_URL?
There was a problem hiding this comment.
it's the lando-api host url - I thought we might use it for something else.
There was a problem hiding this comment.
Discussed on IRC, I'm fine with either keeping the variable as-is or with changing it.
There was a problem hiding this comment.
I think HOST_URL works well, imo.
landoapi/spec/swagger.yml
Outdated
| description: Revision does not exist | ||
| description: | ||
| Revision does not exist or its request_id doesn't correspond to | ||
| the saved one |
There was a problem hiding this comment.
Probably best not to mention internal implementation details here. I don't see a request_id mentioned in the POST parameters, either?
There was a problem hiding this comment.
right - it should stay as it was before
request_id is mentioned as "in body" parameter
landoapi/spec/swagger.yml
Outdated
| post: | ||
| operationId: landoapi.api.landings.update | ||
| description: | | ||
| Sends request to transplant service and responds just the status code |
There was a problem hiding this comment.
Typo, "responds with just the status code"
Added POST API endpoint `/landings/{landing_id}/update`.
`landing_id` (from path) and `request_id` (from data) have to correspond
with database entry, otherwise 404 is raised.
Modified the schema by adding error and result.
Added POST API endpoint
/landings/{landing_id}/update.landing_id(from path) andrequest_id(from data) have to correspondwith database entry, otherwise 404 is raised.
Modified the schema by adding error and result.