Add API endpoint to get a Phabricator Revision from Lando #4

purelogiq · 2017-05-30T14:45:27Z

(Note: this is a larger PR and it may help to view the changes for each commit separately)

Refactor tests to reduce duplication

The stubs to create the app and versionfile in tests/test_app.py would also be needed in every other test file that wanted to test the Flask app. This commit extracts those utility functions into their own file which can be imported into any test that needs them. This commit also renames 'test_app.py' to 'tests_dockerflow.py' to better represent what that file is testing.
This is based on what David Walsh did in mozilla-conduit/lando-ui, thanks!

Add API enpoint to query a revision using the new Phabricator Client.

This commit adds a utility class found in phabricator_client.py that abstracts the interface to Phabricator's Conduit API. Currently it supports querying revisions, users, and repos. As Phabricator's API changes, we can take care of those changes in this class without having to rewrite all of our other methods and api endpoints.

Using the newly created class, this commit also adds a new api endpoint /revisions/{id} which returns details about a revision given an id. See the updated spec/swagger.yml for more details about this endpoint.

Add tests for the PhabricatorClient and /revisions/{id} endpoint

This commit adds unit tests for the PhabricatorClient and /revisions/{id} endpoint using the requests_mock library to stub out Phabricator's API responses.

A new folder, canned_responses is added to contain example responses returned by the different services we depend on in our unit tests.

How to test this PR

First run docker-compose up --build to refresh your images.
You can run invoke test to test the updated tests.
Then run docker-compose up again to start the API server. Once running, visit your-docker-ip:8888 which will take you to the OpenAPI index page which allows you to test out the api directly. Currently, the project is setup to query the Phabricator instance at https://mozphab.dev.mozaws.net/. You can test querying https://mozphab.dev.mozaws.net/D1 by inputting 1 for the id field and your phabricator API key (on the mozphab.dev instance). Inspect the result to confirm it is all working properly.

(This PR is now complete)

darkwing · 2017-05-30T14:52:08Z

landoapi/api/revisions.py

+    phab = Phabricator(host=os.getenv('PHABRICATOR_URL'),
+                       token=os.getenv('PHABRICATOR_API_KEY'))
+    phab.update_interfaces()
+    return phab


darkwing · 2017-05-30T14:53:00Z

landoapi/api/revisions.py

+    id_num = id.strip().replace('D', '')
+    phab = get_phabricator_client()
+    response = phab.differential.query(ids=[id_num])
+    if response and response[0]:


Let's call this "revisions"; response makes me think of a network request, which it may be, but seems like it could be confusing.

darkwing · 2017-05-30T14:53:31Z

landoapi/conduit.py

+
+
+def get_revision(id=None, phid=None, **kwargs):
+    id_num = id


Let's use revision_id

The stubs to create the app and versionfile in tests/test_app.py would also be needed in every other test file that wanted to test the Flask app. This commit extracts those utility functions into their own file which can be imported into any test that needs them. This commit also renames 'test_app.py' to 'tests_dockerflow.py' to better represent what that file is testing.

purelogiq · 2017-05-31T14:06:12Z

This PR has changed significantly and will require a new fresh review.
One big change is that I decided against using the disqus phabricator library because we otherwise wouldn't be able to cleanly test this with a library like requests mock. Also it reduces our dependencies and gives us more control over how things work.

purelogiq · 2017-05-31T14:09:36Z

This PR also assumes we will be passing the user's api key instead of using Oauth + an api key specifically for Lando. We cannot do the latter because Phabricator doesn't provide a way to actually check a user's permission to see a revision via its API.

darkwing · 2017-05-31T14:16:53Z

landoapi/api/revisions.py

+from landoapi.phabricator_client import PhabricatorClient
+
+
+def get(api_key, id):


Please don't hate me but can we make this param revision_id? id isn't descriptive and requires searching to find out what it represents

purelogiq · 2017-06-01T21:54:35Z

Updated this with tests, highly recommend that you view the diff by commits instead of the whole thing at once. See the PR summary for how to test all this.

mars-f

That's some clean code, nice work! We just need to address the potential OSS license mixing.

mars-f · 2017-06-02T01:24:36Z

landoapi/phabricator_client.py

@@ -0,0 +1,109 @@
+# This Source Code Form is subject to the terms of the Mozilla Public


Is this the file from python-phabricator? That project says it's licensed under Apache 2.0. If this code is from that project, then our project becomes a mixed-license work (not a bad thing), which means we should include a NOTICE file in the project root that contains the Apache 2.0 license text.

No, as you see in the comments above I decided against using any library since making the requests ourselves was actually pretty simple and it makes our testing story a lot easier with requests-mock. So this is a file that I wrote.

mars-f · 2017-06-02T02:08:30Z

tests/test_phabricator_client.py


-def test_get_revision_returns_200():
+
+def test_get_revision_with_200_response():
    phab = PhabricatorClient(api_key='api-key')
    api_url = '%s/api/differential.query' % os.getenv('PHABRICATOR_URL')


Part of the api_url line can be refactored into a fixture:

@pytest.fixture def base_url(): return '{}/api/'.format(os.getenv('PHABRICATOR_URL)) def test_myapi(base_url): api_url = base_url + 'foo.query'

mars-f

Looking good! Just some minor style cleanup.

mars-f · 2017-06-02T19:11:35Z

landoapi/phabricator_client.py

+        result = self.__get('/phid.query', {'phids[]': [phid]})
+        return result.get(phid) if result else None
+
+    def __request(self, url, data=None, params=None, method='GET'):


This method name should be preceded by a single underscore.

mars-f · 2017-06-02T19:25:01Z

landoapi/phabricator_client.py

+
+        return response.get('result')
+
+    def __get(self, url, data=None, params=None):


This method name should be preceded by a single underscore. I also find overloading the name get() a bit of a mind twist - doing so is rare in Python because of get's special status. You might want to use a different method name, like _do_get() or _GET().

mars-f · 2017-06-02T19:25:49Z

landoapi/phabricator_client.py

+    def __get(self, url, data=None, params=None):
+        return self.__request(url, data, params, 'GET')
+
+    def __post(self, url, data=None, params=None):


Same as above, should be single underscore.

mars-f · 2017-06-02T19:28:21Z

landoapi/phabricator_client.py

+import requests
+
+
+class PhabricatorAPIException(Exception):


Exception definitions should be at the bottom of the file, after classes.

darkwing

Just one small comment but looks great!

darkwing · 2017-06-05T16:38:20Z

landoapi/phabricator_client.py

+            result = self._GET('/differential.query', {'ids[]': [id_num]})
+        elif phid:
+            result = self._GET('/differential.query', {'phids[]': [phid]})
+        else:


Instead of this else, can you just initialize result to None? Wont the return ternary take care of the return value?

This commit adds a utility class found in phabricator_client.py that abstracts the interface to Phabricator's Conduit API. Currently it supports querying revisions, users, and repos. As Phabricator's API changes, we can take care of those changes in this class without having to rewrite all of our other methods and api endpoints. Using the newly created class, this commit also adds a new api endpoint /revisions/{id} which returns details about a revision given an id. See the updated spec/swagger.yml for more details about this endpoint.

This commit adds unit tests for the PhabricatorClient and /revisions/{id} endpoint using the requests_mock library to stub out Phabricator's API responses. A new folder, `canned_responses` is added to contain example responses returned by the different services we depend on in our unit tests.

darkwing reviewed May 30, 2017

View reviewed changes

purelogiq force-pushed the plq/revision-api branch from cd3b036 to e19fec3 Compare May 31, 2017 12:20

purelogiq force-pushed the plq/revision-api branch from e19fec3 to 631991d Compare May 31, 2017 13:54

purelogiq changed the title ~~[WIP] Lando-Phab revision API endpoints~~ Lando-Phab revision API endpoints May 31, 2017

purelogiq changed the title ~~Lando-Phab revision API endpoints~~ Add API endpoint to get a Phabricator Revision from Lando May 31, 2017

darkwing reviewed May 31, 2017

View reviewed changes

purelogiq requested a review from mars-f June 1, 2017 15:21

purelogiq force-pushed the plq/revision-api branch 2 times, most recently from a7a6d5d to b6fb5ca Compare June 1, 2017 21:53

mars-f suggested changes Jun 2, 2017

View reviewed changes

purelogiq force-pushed the plq/revision-api branch from b6fb5ca to dabac5d Compare June 2, 2017 17:47

mars-f suggested changes Jun 2, 2017

View reviewed changes

purelogiq force-pushed the plq/revision-api branch 2 times, most recently from 1496b8e to 1df11fc Compare June 2, 2017 19:53

darkwing reviewed Jun 5, 2017

View reviewed changes

purelogiq added 2 commits June 5, 2017 12:58

purelogiq force-pushed the plq/revision-api branch from 1df11fc to 3485025 Compare June 5, 2017 16:58

mars-f approved these changes Jun 5, 2017

View reviewed changes

purelogiq merged commit c670fc1 into master Jun 5, 2017

purelogiq deleted the plq/revision-api branch June 7, 2017 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API endpoint to get a Phabricator Revision from Lando #4

Add API endpoint to get a Phabricator Revision from Lando #4

purelogiq commented May 30, 2017 •

edited

Loading

darkwing May 30, 2017

darkwing May 30, 2017

darkwing May 30, 2017

purelogiq commented May 31, 2017

purelogiq commented May 31, 2017

darkwing May 31, 2017

purelogiq commented Jun 1, 2017

mars-f left a comment

mars-f Jun 2, 2017

purelogiq Jun 2, 2017

mars-f Jun 2, 2017

purelogiq Jun 2, 2017

mars-f left a comment

mars-f Jun 2, 2017

mars-f Jun 2, 2017 •

edited

Loading

mars-f Jun 2, 2017

mars-f Jun 2, 2017

darkwing left a comment

darkwing Jun 5, 2017

		from landoapi.phabricator_client import PhabricatorClient


		def get(api_key, id):

		@@ -0,0 +1,109 @@
		# This Source Code Form is subject to the terms of the Mozilla Public


		return response.get('result')

		def __get(self, url, data=None, params=None):

Add API endpoint to get a Phabricator Revision from Lando #4

Add API endpoint to get a Phabricator Revision from Lando #4

Conversation

purelogiq commented May 30, 2017 • edited Loading

Refactor tests to reduce duplication

Add API enpoint to query a revision using the new Phabricator Client.

Add tests for the PhabricatorClient and /revisions/{id} endpoint

How to test this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purelogiq commented May 31, 2017

purelogiq commented May 31, 2017

Choose a reason for hiding this comment

purelogiq commented Jun 1, 2017

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

mars-f left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mars-f Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkwing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purelogiq commented May 30, 2017 •

edited

Loading

mars-f Jun 2, 2017 •

edited

Loading