first commit #2

odra · 2020-03-31T17:01:42Z

first or which uses the bravado library
instructions on usage in the readme file

Sample usage:

c = Client.from_url('http://myspecurl.com', base_url='http://fasjson.example.test/fasjson' principal='admin@EXAMPLE.TEST')

output = c.api.v1.whoami().response().result

print(output)

I couldn't find an easy way to handle API call errors so I think we will have to stick with the bravado exceptions for now.

odra · 2020-03-31T17:02:20Z

I am taking a look on how to generate api docs using sphinx but the code should be in good shape for review.

abompard

Thanks! I have a few comments if we're going this route.

Also, please add a tox.ini file that checks the unit tests, black, flake8, bandit, licenses and code coverage.

abompard · 2020-03-31T18:01:10Z

README.md

+```
+c = Client.from_url('http://myspecurl.com', base_url='http://fasjson.example.test/fasjson' principal='admin@EXAMPLE.TEST')
+
+output = c.api.v1.whoami().response().result


Could we propose something more direct? Like c.whoami(), ideally.

is just uses whatever is in the url path, so /v1/whoami becomes v1.whoami(), to use whoami()directly we need to remove v1 path from the url.

Maybe not removing from the server, but only hide it in the client? It feels like our users shouldn't have to know what boils down to an implementation detail. Same thing for the .response().result part. I'm all ears if you have a different opinion.

I meant this is auto generated by bravado, the /v1/whoami thing is read from the api spec... I could do some method naming fu using "_getarr" to retrieve/call methods within the v1 object.. but it doesn't feel right, I don't jhave a problem with "v1", we could remove it from the spec since we only have a version of the api now.

About response().result that bravado again because response() returns a future so you can handle async code if you want to... I could try to use the getattr method here again.

github messed up with my comment, I wanted to write __ getattr __ :P

an alternative would be to have different specs per api version and add the /v1 part in the basePath field.

abompard · 2020-03-31T18:02:43Z

README.md

+## Usage
+
+```
+c = Client.from_url('http://myspecurl.com', base_url='http://fasjson.example.test/fasjson' principal='admin@EXAMPLE.TEST')


I think we should have the API server serve the spec at some known endpoint, and have the client automatically pull it from there. End users shouldn't have to know where we publish the spec file.

right, this is is just a dummy readme file for now, we could host the spec definition in the root endpoint (/v1)

I think we could have a default value that could be changed if needed but the value itself needs to be changed once we add this feature in the api

abompard · 2020-03-31T18:04:01Z

README.md

+Install dependencies:
+
+```
+pip install -U -r requirements.txt


A real python package would be better. Could you wite a setup.py file or a pyproject.toml file?

yep, I just sent it like this for an early feedback, "development" here stand for people who want to contribute to the repository.

can you send me offline (irc, email etc) what values should I use for author name, email, etc.

You can have a look at noggin and / or fasjson if you need inspiration, that's what I would do ;-)

Any news on that item?

abompard · 2020-03-31T18:04:23Z

README.md

+Running tests:
+
+```
+pytest -s


Do we need the -s flag?

I always used "-s" so pytest won't hide any output from the tests (such as data sent to stdout), I can remove it

abompard · 2020-03-31T18:04:44Z

README.md

+
+## License
+
+Licensed under [GPLv3](./LICENSE)


We may want to consider LGPL, as it's a library.

sure, I just used what we used in fasjson

abompard · 2020-03-31T18:07:53Z

fasjson_client/client.py

+    return super(HttpClient, self).authenticated_request(request_params)
+
+
+class Client(object):


In python3 you don't need to derive from object.

abompard · 2020-03-31T18:09:23Z

fasjson_client/client.py

+        }
+      raise errors.ClientError('error loading remote spec data', errno.ECONNABORTED, data=data)
+
+    if res.status_code != 200:


You can check the res.ok attribute, it'll cover all the 2xx range.

abompard · 2020-03-31T18:12:10Z

fasjson_client/errors.py

+    self.data = data
+
+  def __repr__(self):
+    return f'<{self.__class__.__name__} code={[self.code]} message={self.message} data={self.data}>'


Do you want the code to appear as a list? If the brackets are only for decoration then I think they should be outside the curly braces.

ah not really, it was a mistake of mine, I will remove it

abompard · 2020-03-31T18:12:40Z

fasjson_client/errors.py

+
+class ClientError(BaseError):
+  def __init__(self, message, code, data=None):
+    super(ClientError, self).__init__(message, code, data=data)


Same here, in Python 3 only super() is required.

abompard · 2020-03-31T18:14:07Z

requirements.txt

@@ -0,0 +1,38 @@
+attrs==19.3.0


Please only list the dependencies that are directly required, and don't mandate the versions unless it's actually necessary. Otherwise it makes it impossible to integrate in an application that uses the system python.

pypingou · 2020-04-01T15:54:39Z

The commit messages may need some work, the first commit mentions testing files but changes the LICENSE file for example.

abompard · 2020-04-01T18:36:43Z

setup.py

+here = os.path.abspath(os.path.dirname(__file__))
+
+
+def content(path, cwd=None):


Why is the cwd argument sometimes necessary?

I was having issues in reading the README file vs the requirements one, it was not able to read the readme file withtout the current working dir var.

OK but why not always use here when building the paths, then? This way youd have one less parameter and you'll know all paths are in the same directory as this file (I like removing things ;-) )

abompard · 2020-04-01T18:38:50Z

tox.ini

+commands =
+    rm -rf htmlcov coverage.xml
+    py.test -vv --cov-config .coveragerc --cov=fasjson_client \
+        --cov-report term --cov-report xml --cov-report html test/


You can add {posargs} at the end so we can add flags at runtime

abompard · 2020-04-01T18:40:46Z

setup.py

+    ],
+    license="LGPLv3",
+    install_requires=content('requirements.txt').split('\n'),
+    tests_requires=content('test_requirements.txt').split('\n'),


Are you sure this works, with the empty lines and the comments in that file? I think they should be filtered out by the content function.

fasjson_client/client.py

abompard · 2020-04-01T18:44:24Z

fasjson_client/client.py

+      raise errors.ClientError("schema validation failed", errno.EPROTO)
+
+  @classmethod
+  def from_url(cls, spec_url, base_url=None, principal=None):


Let's go like this for now, but eventually I'd prefer the spec URL to be deducted from the base URL (maybe a /spec endpoint? Or a header? Is there a standard for that?)

spec_url is where bravado loads the spec from including the base url whcih is documented in the api spec file, base_url is here in case a user wants do overwrite the value used within the spec.

we can have a default value for the spec url but I think we need to push fasjson somewhere first?

If we decide that fasjson publishes the spec at /spec for example (again, if there's a standard location for that, let's use it), then we can just provide a base URL and the client will know where to find the spec.

I created an issue to provide it in the root version path, such as /v1, so I think we can update it once we have it hosted somewhere/defined a production baseurl.

Has it been merged yet? If not could you link to the issue here so we can track it?

I created an issue but didn't worked on it - it needs to implement some kind of "white list" which enables us to choose which path should not be authenticated against ldap.

We could set the url from oour git repository while for now if that's ok.

OK, let's go that way for now.

abompard · 2020-04-01T18:45:47Z

.coveragerc

+
+
+[report]
+fail_under = 88


Let's start with that but not wait too long to go to 100%, it's harder to do it after the fact.

I just used an existing tox.ini file from another fedora-infra repository, that's what was being used there.

OK, could you set it to 100% then? Thanks

abompard · 2020-04-01T18:46:23Z

tox.ini

+envlist = py36,py37,diff-cover,lint,format
+
+[testenv]
+passenv = TRAVIS TRAVIS_*


Are you setting up Travis?

abompard

I still have a few comments, thanks.

abompard · 2020-04-15T12:37:14Z

.coveragerc

+
+
+[report]
+fail_under = 88


OK, could you set it to 100% then? Thanks

abompard · 2020-04-15T12:39:23Z

README.md

+Install dependencies:
+
+```
+pip install -U -r requirements.txt


Any news on that item?

abompard · 2020-04-15T12:39:44Z

fasjson_client/client.py

+from requests_gssapi import HTTPSPNEGOAuth
+from bravado import requests_client
+from bravado.client import SwaggerClient
+import gssapi


That comment is still valid I think.

fasjson_client/client.py

abompard · 2020-04-15T12:41:20Z

fasjson_client/client.py

+      raise errors.ClientError("schema validation failed", errno.EPROTO)
+
+  @classmethod
+  def from_url(cls, spec_url, base_url=None, principal=None):


Has it been merged yet? If not could you link to the issue here so we can track it?

abompard · 2020-04-15T12:44:15Z

setup.py

+here = os.path.abspath(os.path.dirname(__file__))
+
+
+def content(path, cwd=None):


OK but why not always use here when building the paths, then? This way youd have one less parameter and you'll know all paths are in the same directory as this file (I like removing things ;-) )

abompard · 2020-04-15T12:48:06Z

test/conftest.py

+
+@pytest.fixture
+def fixture_dir(test_dir):
+    return f"{test_dir}/fixtures"


Ideally we would join paths with os.path.join() instead of hardcoding slashes (except for URLs of course), but I agree it does not make much of a difference since we're never going to run it on Windows ;-)

abompard · 2020-04-15T12:49:51Z

test_requirements.txt

+requests
+requests-mock
+
+# required by vcs looks like


do you mean vcr?

Signed-off-by: Aurélien Bompard <aurelien@bompard.org>

abompard · 2020-04-20T15:45:03Z

Merging now so others can start working on it.

abompard · 2020-04-21T06:39:44Z

This is related to fedora-infra/fasjson#8.

first commit

80cdabf

odra requested a review from abompard March 31, 2020 17:01

abompard requested changes Mar 31, 2020

View reviewed changes

odra force-pushed the poc3 branch 3 times, most recently from c91806d to 5feb612 Compare April 1, 2020 13:41

odra added 2 commits April 1, 2020 13:34

testing files

c914d83

update license and readme files

3747ce1

odra force-pushed the poc3 branch from 5feb612 to 3747ce1 Compare April 1, 2020 16:35

abompard requested changes Apr 1, 2020

View reviewed changes

odra mentioned this pull request Apr 6, 2020

swagger spec fedora-infra/fasjson#27

Merged

abompard added this to Ready for review in AAA via automation Apr 7, 2020

odra added 3 commits April 7, 2020 08:17

remove travis

dc790f7

add {posargs} in tox.ini file

12b0620

requirements file parsing

c3e57de

abompard requested changes Apr 15, 2020

View reviewed changes

odra and others added 7 commits April 16, 2020 08:53

code coverage set to 100

56771b5

wrap bravado exception in ClientError

e909a3b

adding SPEC_URL var

a574f8d

update tests

60ebe93

Use Poetry

0d971d2

Signed-off-by: Aurélien Bompard <aurelien@bompard.org>

Minor fixes

f0f5171

Signed-off-by: Aurélien Bompard <aurelien@bompard.org>

Adapt tox to poetry and add more checks

b8c0655

Signed-off-by: Aurélien Bompard <aurelien@bompard.org>

abompard merged commit fa67891 into fedora-infra:master Apr 20, 2020

AAA automation moved this from Ready for review to Done within Sprint Apr 20, 2020

abompard removed this from Done within Sprint in AAA Apr 21, 2020

		return super(HttpClient, self).authenticated_request(request_params)


		class Client(object):

		here = os.path.abspath(os.path.dirname(__file__))


		def content(path, cwd=None):



		[report]
		fail_under = 88



		[report]
		fail_under = 88

first commit #2

first commit #2

Conversation

odra commented Mar 31, 2020 • edited

odra commented Mar 31, 2020

abompard 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

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

odra Mar 31, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pypingou commented Apr 1, 2020

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

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

abompard 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

abompard commented Apr 20, 2020

abompard commented Apr 21, 2020

odra commented Mar 31, 2020 •

edited

odra Mar 31, 2020 •

edited