Check the status of landing jobs #9

zalun · 2017-06-14T13:13:56Z

No description provided.

zalun · 2017-06-14T14:33:54Z

landoapi/api/transplants.py

+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+"""
+Revision API


Transplant API :)

zalun · 2017-06-14T14:34:28Z

landoapi/api/transplants.py

+    return transplant.json(), 200
+
+
+def _format_revision(


This should be removed

purelogiq · 2017-06-19T06:04:10Z

docker-compose.yml

@@ -18,6 +18,7 @@ services:
      - PHABRICATOR_URL=https://mozphab.dev.mozaws.net
      - PHABRICATOR_UNPRIVILEGED_API_KEY=api-123456789
      - TRANSPLANT_URL=https://stub.transplant.example.com
+      - DATABASE_URL=sqlite:////sqlite.db


Not really an issue now, but, at some point we should start using postgres in the development environment since that what will be used in production.

purelogiq · 2017-06-19T06:10:22Z

landoapi/api/revisions.py



-def get(revision_id, api_key=None):
-    """ API endpoint at /revisions/{id} to get revision data. """
+def get_revision(revision_id, api_key=None):


I get the idea behind this method, but, in my view it would be better to keep this file to 2 things: 1) the api functions for revisions, 2) helper methods for only those api functions.

If the transplants.py needs to get revisions, create a _get_revision method within that file specific to its needs.

purelogiq · 2017-06-19T06:11:36Z

landoapi/api/revisions.py


-    return _format_revision(phab, revision, include_parents=True), 200
+    return _format_revision(phab, revision, include_parents=True)


If you do make a helper method for transplants.py remember that setting include_parents=True is very expensive because it will make several network requests. In fact, if you only need to get the revision id, then you won't need to call_format_revision at all.

purelogiq · 2017-06-19T06:12:21Z

landoapi/api/revisions.py

-    revision = phab.get_revision(id=revision_id)
+def get(revision_id, api_key=None):
+    """ API endpoint at /revisions/{id} to get revision data. """
+    revision = get_revision(revision_id, api_key)


Let's revert this and have transplants.py use its own private helper method.

purelogiq · 2017-06-19T06:19:45Z

landoapi/app.py

    return app


+def create_tables():


This function is never used (?)

purelogiq · 2017-06-19T06:32:19Z

landoapi/api/transplants.py

+
+
+def check_status(transplant_id):
+    """ API endpoint at /revisions/{id}/transplants to list landing jobs


2 things.

This comment doesn't reflect what this function does. The comment says it lists all landing jobs for a revision, but, infact it only retrieves one landing job based on a given id.

Like I mentioned in the last PR. Having an endpoint to get an individual transplant job based on its id will serve no purpose. This is because external services (e.g. Lando UI) should not (or in the case of Lando UI literally cannot) save the transplant ids. The only way for them to query the status of a landing is by revision id.

I propose these endpoints
/revisions/{id}/land - This lands a revision like you have already implemented. The change here is to remove transplants from the url because it doesn't really matter for external clients to know the specifics.

/revisions/{id}/landings - For each transplant job on that revision returns: ldap username of person that started it, date started, date finished, landing status and target repo.

This is just a proposal, but, I think we should have a discussion about it @zalun & @mars-f.

I'd drop the "land" endpoint and just POST directly to /revision/{id}/landings.

•smacleod> mars (/whoever is touching transplant): please see https://docs.google.com/document/d/1cRIdpVqYcn0Mle7vyjJePNw5TcFoZ20N42dLVy5xVEk/edit#heading=h.lteitxoe85mb 2:26:04 PM mars: as someone who has been bit by nesting api endpoints, I highly advise against it (CC imadueme) 2:26:14 PM (re the landings stuff above) 2:26:56 PM e.g. go with /landings/?revision=<blah> vs /revisions/<blah>/landings 2:27:10 PM <mars> smacleod: so you would rather have us post to /revision/{id}/landings, and that takes you to /landings/{landing_id} ? 2:27:13 PM <•smacleod> keep your ids global, endpoints flat, with query arguments to filter

smacleod proposes that we do not use nested api endpoints.

Get revision data /revisions ?=id ?=api-key Land a revision /revisions/land ?=revision_id ?=api-key Return all the current and past landings for a revision /landings ?=revision_id ?=api-key

I propose this.

Revised:

All endpoints can have an optional Phabricator API Key in Request Header (which will be used when working with secure revisions.) Get revision data GET /revisions ?=id Return all the current and past landings for a revision GET /landings ?=revision_id ?=filter=(all,inprogress,failed,etc) Land a revision POST /landings ?=revision_id

Revised 😤 😃

All endpoints can have an optional Phabricator API Key in Request Header (which will be used when working with secure revisions.) Get revision data GET /revisions/{id} Return all the current and past landings for a revision GET /landings ?=revision_id ?=status=(all,inprogress,failed,etc) Land a revision POST /landings Body: revision_id

@purelogiq I moved the design bit to https://public.etherpad-mozilla.org/p/lando-api-urls

purelogiq · 2017-06-19T06:35:32Z

landoapi/models/transplant.py

+    id = db.Column(db.Integer, primary_key=True)
+    request_id = db.Column(db.Integer, unique=True)
+    revision_id = db.Column(db.String(30))
+    status = db.Column(db.Integer)


Personally I feel using a string here would be much better, e.g 'started', and 'finished'.
In the API it is not clear what 0 or 2 means without looking at the source code, for an external client that is very confusing. In addition if we ever have to debug the database it is nice to see text of the statuses instead of a number.

What do you think?

Agreed - I was also thinking that it should be changed.

For the job statuses I suggest we copy the taskcluster task statuses. "pending", "running", and "completed", "failed", and possibly "exception".

purelogiq · 2017-06-19T06:36:55Z

tests/test_revisions.py

@@ -3,8 +3,12 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 from urllib.parse import parse_qs

+import json


The imports here on line 6, 9, and 11 are never used (?)

yes - leftover

purelogiq · 2017-06-19T06:37:52Z

tests/test_transplants.py

+class TestTransplants:
+    def setup_method(self):
+        conn = _app.create_app('/version.json')
+        with conn.app.app_context():


I actually think this is ok, or at least not something we have to worry about for now. Hopefully we find a solution soon :)

https://github.com/kevinbeaty/flask-todomvc/blob/master/tests/todos_tests.py#L17 does something similar.

cool, I've been also asking around and people are saying it's OK

purelogiq · 2017-06-19T06:39:53Z

I also tested this in the browser and it worked! So I think we're on the right track.

TransplantClient is created to handle the requests. It is using request-mock module. After connecting to the real server only this class and tests should be changed. POST request to `/requests/{request_id}/transplants` is calling creating an autoland job using the data taken from Phabricator.

purelogiq

Some comments, but looks good to land to me :)

purelogiq · 2017-06-21T15:07:47Z

landoapi/models/landing.py

+    }
+
+
+class Landing(db.Model):


We should store the ldap_username, date_started, and date_landed. We can make another card for it so that we can land this one quicker.

purelogiq · 2017-06-21T15:09:13Z

landoapi/spec/swagger.yml

      request_id:
        type: integer
        description: |
-          The id of the request in Transplant service
+          The id of the Request in Transplant service


I don't think we should expose the transplant request_id to the public. What do you think?

It's not needed, might be useful for debugging.

mars-f

The structure looks good; some fixups requested for style, comments and such.

mars-f · 2017-06-21T15:10:38Z

docker/Dockerfile-dev

@@ -4,7 +4,10 @@

 FROM python:3.5-alpine

+RUN apk --update --no-cache add \
+	sqlite > /dev/null 2>&1


I haven't seen the bit about coyping stderr to stdout done in Dockerfiles before, especially for 'apk add'. Is that a best practice?

copied blindly from another Dockerfile, I'll remove it

mars-f · 2017-06-21T15:12:07Z

docker/Dockerfile-dev

 ADD requirements.txt /requirements.txt
+ADD docker/db/schema.db /sqlite.db


Do we need to worry about database file permissions? In prod the application server runs under a different user.

This is not ready for production - I guess it would rather link the directory. Also manager is needed to run create_all on an empty database.

mars-f · 2017-06-21T15:19:01Z

landoapi/api/landings.py

+        return problem(
+            502,
+            'Landing not created',
+            'The requested revision does exist, but landing failed',


It would be nice if the error message said something like "Please retry your request at a later time."

mars-f · 2017-06-21T15:27:19Z

landoapi/models/landing.py

+from landoapi.phabricator_client import PhabricatorClient
+from landoapi.transplant_client import TransplantClient
+
+TRANSPLANT_JOB_STARTED = 'started'


Could you add a comment explaining that these strings are known values copied from the Transplant service API?

mars-f · 2017-06-21T15:28:46Z

landoapi/models/landing.py

+        """ Land revision and create a Transplant item in storage. """
+        revision = _get_revision(revision_id, phabricator_api_key)
+        if not revision:
+            raise RevisionNotFoundException()


We will need the missing Revision's ID in the exception message for debugging.

mars-f · 2017-06-21T15:31:17Z

landoapi/models/landing.py

+    @classmethod
+    def get(cls, landing_id):
+        """ Get Landing object from storage. """
+        t = cls.query.get(landing_id)


Same, could you please expand this variable name?

mars-f · 2017-06-21T15:32:52Z

landoapi/models/landing.py

+        return '<Landing: %s>' % self.id
+
+    def serialize(self):
+        """ Serialize """


Docstring needs more detail. What target format are we serializing into? JSON compatible? sqlite compatible?

mars-f · 2017-06-21T15:34:01Z

landoapi/models/landing.py

+
+
+class LandingNotCreatedException(Exception):
+    pass


Needs a docstring to explain that this exception represents downstream service errors.

mars-f · 2017-06-21T15:34:56Z

landoapi/models/landing.py

+    pass
+
+
+class LandingNotFoundException(Exception):


Needs a docstring to explain that this exception represents a specific downstream service response (I think?).

mars-f · 2017-06-21T15:47:04Z

tests/test_landings.py

+@pytest.mark.usefixtures('client_class')
+@pytest.mark.usefixtures('phabfactory_class')
+@pytest.mark.usefixtures('docker_env_vars')
+class TestLanding:


These tests should be converted from xUnit-style to pytest-style.

mars-f

LGTM, just needs some minor tweaks and it can land.

mars-f · 2017-06-22T14:08:42Z

tests/test_landings.py

+        _db.drop_all()
+
+
+def test_landing_revision(db, client, phabfactory):


This test name should be something like test_landing_request_is_saved_to_database().

mars-f · 2017-06-22T14:09:44Z

docker/Dockerfile-prod

@@ -7,6 +7,10 @@ FROM python:3.5-alpine
 RUN addgroup -g 1001 app && \
    adduser -D -u 1001 -G app -s /usr/sbin/nologin app

+RUN apk --update --no-cache add \
+	sqlite > /dev/null 2>&1


The 2>&1 should be removed to match Dockerfile-dev.

mars-f · 2017-06-22T14:10:18Z

tests/test_landings.py

+
+@pytest.fixture
+def db(app):
+    """Monkeypatch environment variables that we'd get running under docker."""


Docstring needs rewording, it doesn't match the fixture's purpose.

mars-f · 2017-06-22T14:10:41Z

tests/test_landings.py

+    with app.app_context():
+        _db.init_app(app)
+        _db.create_all()
+        # Yield the connexion app so other fixtures can get the app instance


Comment needs removal.

Renamed Transplant to Landing in API, models, db schema. Current API: - /landings POST - land the revision GET - get a list by revision and/or status - /landings/{id} GET - get a specific landing Added `manage.py` file with a `create_db` command to create a database on the container. It is linked in docker-compose to `./.db` directory.

zalun force-pushed the check_land_job_state branch from da561ec to 5990c01 Compare June 14, 2017 14:31

zalun commented Jun 14, 2017

View reviewed changes

landoapi/api/transplants.py Outdated

return transplant.json(), 200

def _format_revision(

Copy link

Contributor Author

zalun Jun 14, 2017

Choose a reason for hiding this comment

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

This should be removed

zalun force-pushed the check_land_job_state branch 6 times, most recently from f76529e to 6013235 Compare June 16, 2017 12:11

purelogiq suggested changes Jun 19, 2017

View reviewed changes

zalun force-pushed the check_land_job_state branch from 6013235 to 835f35f Compare June 20, 2017 16:20

zalun force-pushed the check_land_job_state branch 2 times, most recently from 253bdc5 to 41a5363 Compare June 21, 2017 14:15

zalun changed the title ~~WIP Check land job state~~ Check the status of landing jobs Jun 21, 2017

zalun force-pushed the check_land_job_state branch from 41a5363 to 536e433 Compare June 21, 2017 14:18

purelogiq approved these changes Jun 21, 2017

View reviewed changes

mars-f suggested changes Jun 21, 2017

View reviewed changes

zalun force-pushed the check_land_job_state branch 2 times, most recently from cea0777 to 599038c Compare June 22, 2017 12:45

mars-f approved these changes Jun 22, 2017

View reviewed changes

zalun force-pushed the check_land_job_state branch from 599038c to fb33fcc Compare June 22, 2017 15:06

zalun merged commit 6d48c6f into mozilla-conduit:master Jun 23, 2017


		return _format_revision(phab, revision, include_parents=True), 200
		return _format_revision(phab, revision, include_parents=True)



		def check_status(transplant_id):
		""" API endpoint at /revisions/{id}/transplants to list landing jobs

		ADD requirements.txt /requirements.txt
		ADD docker/db/schema.db /sqlite.db

		_db.drop_all()


		def test_landing_revision(db, client, phabfactory):

Check the status of landing jobs #9

Check the status of landing jobs #9

Conversation

zalun commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purelogiq Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purelogiq commented Jun 19, 2017

purelogiq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mars-f left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mars-f left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purelogiq Jun 19, 2017 •

edited

Loading