Setup Python client PoC #2

lampajr · 2024-03-19T11:44:33Z

First draft of a possible python client library of Horreum.

Autogenerated client code using kiota (we can switch to other generators as well)
Automate the client code generation in a Github workflow
Setup simple CI to check that the build works
Setup simple integration tests that require Horreum instance up and running in the background (not automated yet)
No high level api at the moment, right now we need to use the underlying (and autogenerated) raw client

Further improvements (that could be done in different PRs if we agree):

Manage project version
Setup release process
Migrate from keycloak to a generic oidc library
Implement high level api (right now we can use the raw api)
Automate Horreum startup for ITs
PR checks with ITs, needs full automation to make this working
Consider using nox for test automation in multipl py envs
Full Horreum API coverage (I would suggest the high level one)

lampajr · 2024-03-19T17:08:33Z

Hey @stalep, I think this this could be a good starting point/PoC that showcase how the python library could be implemented.

For sure there are many improvements and changes required, I tried to list some of them in the description.

webbnh

Hi Andrea, @stalep asked me to look this over. I found a bunch of nits and small stuff which you might want to polish, but generally this looks excellent, although I only reviewed the tests and the infrastructure stuff.

I've commented below on whether the openapi.yml file should be checked into the repo. The same question applies to all of the files which are generated from it. If you'd like to discuss this in the context of the PR, that would be a good place to do so.

Once you settle questions around high-level vs. low-level client (and whether Kiota is to be the generator of choice...we've changed that twice, now... 😉), to your list of todo's I would add an aspiration for full test coverage of the Horreum API. 😁

.github/workflows/ci.yaml

.github/workflows/generate-client.yaml

Makefile

webbnh · 2024-03-19T20:16:50Z

Makefile

+		curl -sLO https://github.com/microsoft/kiota/releases/download/${KIOTA_VERSION}/${OS_NAME}-${OS_ARCH}.zip ;\
+		unzip -o linux-x64.zip ;\


Shouldn't the argument to the unzip command be ${OS_NAME}-${OS_ARCH}.zip?

good catch, thx!

README.md

webbnh · 2024-03-19T20:39:31Z

openapi/openapi.yaml

Are you sure that you want this file committed to this repo? This is an artifact of a Horreum build, and it's an input to this repo's build -- it's not really a "source file".

The same question applies to all of the files generated from the OpenAPI spec: they are "build outputs" not source files, and, as such, they shouldn't really be committed to the repo.

What I would suggest that we need is a way to take the output of the Horreum build, in the form of the OpenAPI spec, generate a Python client from it, build a wheel, and publish the wheel, so that that it can be pulled and installed by consumers. So, the input is the OpenAPI spec, the output is the wheel; this repo would (potentially) house the infrastructure for performing that process, and it might potentially deliver wheels as release artifacts. But, I really don't think it's appropriate to include the OpenAPI spec or any of the generated files in this repo.

(I'm happy to discuss this further!)

That's a good point to discuss, I was in doubt as well.

Based on my experience there are two possible approaches here, either commit all generated sources (including openapi.yaml) or simply generate them during the build of the whl without committing them to the github repo. I think that both have their own pros and cons. Here why I decided to commit everything (at least at the moment):

Committing the openapi.yaml that is used to generated the client let us having good evidence of the source especially if we found issues in previous release, we can reproduce the local build easily.

Right now I couldn't find any particular issue with the auto-generated client, but we don't know if in the future we would have to make some temporary bug fixing to that. Having the generated source code committed could help us in this.

Having everything committed allows us to build the project in disconnected / offline environments, not sure that is really required but nice to have (to be fair just the openapi.yaml is required for this)

➡️ But anyway I do not have a strong opinion on this but, so if you agree I would open a separate issue if we want to discuss further on this.
I mean I think we could decide later if we want to commit everything or not, this decision wouldn't affect the build itself so I would focus on having something working and improve it in further steps.

What I would suggest that we need is a way to take the output of the Horreum build, in the form of the OpenAPI spec, generate a Python client from it, build a wheel, and publish the wheel

IIUC what you are suggesting I am not that sure this is so straightforward as the python client would have to manage the authentication/authorization which is something that cannot be autogenerated from the openapi, therefore we would have to add that as part of this project (e.g., right now I used keycloak but we would have to generalize using oidc library)

I would open a separate issue if we want to discuss further on this.

This sounds more like a "discussion" than an "issue", but, for whatever reason, this repo doesn't seem to have Discussions enabled. So, I've opened #3.

pyproject.toml

webbnh · 2024-03-19T20:54:09Z

test/horreum_client_test.py

+        pytest.fail("Unable to fetch Horreum version, is Horreum running in the background?")
+
+    return client


You've got an extra blank line here that you don't have before the return in the previous function.

It looks like this change wasn't committed.

lampajr

Hey @webbnh , thanks a lot for your review!!
I applied and replied all your comments (I hope I did not miss any).

Once you settle questions around high-level vs. low-level client (and whether Kiota is to be the generator of choice...we've changed that twice, now... 😉), to your list of todo's I would add an aspiration for full test coverage of the Horreum API.

I agree with you, added as additional todo.

Regarding https://github.com/Hyperfoil/horreum-client-python/pull/2/files#r1531078859 I would open a separate issue for discussing it further.

.github/workflows/ci.yaml

.github/workflows/generate-client.yaml

Makefile

lampajr · 2024-03-20T08:20:03Z

Makefile

+		curl -sLO https://github.com/microsoft/kiota/releases/download/${KIOTA_VERSION}/${OS_NAME}-${OS_ARCH}.zip ;\
+		unzip -o linux-x64.zip ;\


good catch, thx!

README.md

pyproject.toml

lampajr · 2024-03-20T09:02:57Z

openapi/openapi.yaml

That's a good point to discuss, I was in doubt as well.

Based on my experience there are two possible approaches here, either commit all generated sources (including openapi.yaml) or simply generate them during the build of the whl without committing them to the github repo. I think that both have their own pros and cons. Here why I decided to commit everything (at least at the moment):

Committing the openapi.yaml that is used to generated the client let us having good evidence of the source especially if we found issues in previous release, we can reproduce the local build easily.

Right now I couldn't find any particular issue with the auto-generated client, but we don't know if in the future we would have to make some temporary bug fixing to that. Having the generated source code committed could help us in this.

Having everything committed allows us to build the project in disconnected / offline environments, not sure that is really required but nice to have (to be fair just the openapi.yaml is required for this)

➡️ But anyway I do not have a strong opinion on this but, so if you agree I would open a separate issue if we want to discuss further on this.
I mean I think we could decide later if we want to commit everything or not, this decision wouldn't affect the build itself so I would focus on having something working and improve it in further steps.

What I would suggest that we need is a way to take the output of the Horreum build, in the form of the OpenAPI spec, generate a Python client from it, build a wheel, and publish the wheel

IIUC what you are suggesting I am not that sure this is so straightforward as the python client would have to manage the authentication/authorization which is something that cannot be autogenerated from the openapi, therefore we would have to add that as part of this project (e.g., right now I used keycloak but we would have to generalize using oidc library)

Co-authored-by: Webb Scales <[email protected]>

lampajr · 2024-03-20T10:23:13Z

Squashed all commits into 486c86c

mfleader · 2024-03-20T14:06:32Z

It seems like all of the generated code in this pull request should instead exist as an artifact in CI rather than live in the repo.

stalep · 2024-03-20T14:11:29Z

It seems like all of the generated code in this pull request should instead exist as an artifact in CI rather than live in the repo.

We have discussed that and yes, that's the ideal, but for now we want to prioritize to get out a client and most consumers would not look at this repo directly but rather download the client over pypi and use it.

webbnh

@lampajr, you addressed nearly all of my previous issues (feel free to mark them as resolved; I don't seem to have the power to do that, here). However, there is one change which looks like it didn't get committed; there were other missing newlines that you didn't note, which I've flagged; and, per your request, I've opened an issue (#3) to discuss what files to commit to the repo. While I was at it, I ran across a few other nits which I've flagged.

Incidentally, I have a request/suggestion for you: once people have started reviewing your PR, please do not squash your branch -- this makes it very hard to see what you have changed since the last round of review. (Yes, GitHub flags the files which have changed, but not the individual lines, which, with new code, means the whole module looks "new" again.) You can squash it when you go to merge it, after the review is complete. (Any time you are sharing a branch with someone else, if you have to "force push" it, you have probably done something which you shouldn't have; we can cope with rebasing branches, but anything else which modifies the history or ancestry should be avoided.)

webbnh · 2024-03-20T14:44:52Z

.github/workflows/generate-client.yaml

+      - run: git --no-pager diff
+      - name: Create Pull Request
+        uses: gr2m/create-or-update-pull-request-action@v1


You might want to dispense with the git diff step: assuming that the PR gets created/updated, the diff will be available there.

Acutally I kept especially for debugging purposes and local checks, I don't see particular issue in keeping that. Do you see any?

The only issues are that the output might (have) be(en) voluminous. This is not a huge problem (it's not my disk space... 😉), and the workflow web page probably hides it behind an "expansion triangle", but things might go better if we didn't capture it and keep it around (since, if we wanted it, it would be in the pull request).

However, now that you're not including the generated files in the repo, you might want to revisit this. This diff (nor the pull request) will not show the changes to the generated files. I don't know whether that is good or bad, but it removes most of the value from generating the diff here. Of course, without the generated files, the size of this diff is unlikely to be large enough to matter...so maybe it's not worth thinking about anymore. 😁

webbnh · 2024-03-20T14:45:51Z

.gitignore

+# Kiota
+**/.kiota.log


Missing newline.

webbnh · 2024-03-20T14:54:26Z

Makefile

+# add tools bin directory
+PATH := $(PROJECT_BIN):$(PATH)


Are you sure you want to do this? (E.g., this is no longer needed for the kiota invocation at line 68.)

yeah makes sense, we can get rid of this and re-introduce if really needed

webbnh · 2024-03-20T15:00:23Z

pyproject.toml

+[tool.pytest.ini_options]
+asyncio_mode = "auto"


Missing newline.

webbnh · 2024-03-20T15:05:42Z

test/horreum_client_test.py

+        pytest.fail("Unable to fetch Horreum version, is Horreum running in the background?")
+
+    return client


It looks like this change wasn't committed.

webbnh · 2024-03-20T16:08:08Z

src/horreum/horreum_client.py

+        if self.__username is not None and self.__password is not None:
+            # Bearer token authentication
+            access_provider = await setup_auth_provider(self.__base_url, self.__username, self.__password)
+            self.auth_provider = BaseBearerTokenAuthenticationProvider(access_provider)
+        else:
+            # Anonymous authentication
+            self.auth_provider = AnonymousAuthenticationProvider()


If both the username and password are None, then we should use anonymous access; however, if the username is not None, then we should try authenticated access, regardless of what the password is, and let Keycloak sort it out; and, if neither of those cases match (i.e., if the username is None and the password is not None), it would probably be better to raise an error than to just ignore the password.

Yeah that sounds good to me, added check (we can discuss further on the exception message, I am not that good 😄 )

It looks like you're good enough! 😁

Whether you want to raise a RuntimeError or a ValueError is probably not a big deal. You also have the option of defining your own subclass of Exception (or you can subclass one of those other two), but I would not necessarily go that route unless you're going to have several exceptions that you want your caller to be able to differentiate among (or unless you want your caller to be able to catch your exception without having to catch all RuntimeError's), but you can improve that later when there is an actual need.

webbnh · 2024-03-20T16:12:00Z

src/horreum/keycloak_access_provider.py

+    def get_allowed_hosts_validator(self) -> AllowedHostsValidator:
+        pass


Wouldn't return None be more expressive than pass?

IIRC pass does not return anything, so it is slighlty different from a return statement.
Anyway to be more expressive I changed to return AllowedHostsValidator(allowed_hosts=[]) which means any host is allowed.

In Python, all functions return something: unless control leaves the function by executing a return (or yield) statement which specifies an explicit value, the function returns a value of None. So, your pass would result in your caller receiving a None value...and that does not conform to your type hint of AllowedHostsValidator.

Now, you could address this by changing the type hint to something like typing.Optional[AllowedHostsValidator] which would permit the None value. Or, you could do as you did and replace the pass with a return with a proper value. (I believe I like your choice better. 😉)

webbnh · 2024-03-20T16:23:46Z

openapi/openapi.yaml

I would open a separate issue if we want to discuss further on this.

This sounds more like a "discussion" than an "issue", but, for whatever reason, this repo doesn't seem to have Discussions enabled. So, I've opened #3.

lampajr

Hey @webbnh

once people have started reviewing your PR, please do not squash your branch -- this makes it very hard to see what you have changed since the last round of review

My bad, I wrongly squashed everything before your final review 😞

Anyway, I think I addressed all of them but the git diff one as I think it does not harm to keep it there for debugging purposes

I've opened an issue (#3) to discuss what files to commit to the repo

Thanks a lot 🙏

lampajr · 2024-03-20T16:42:56Z

.github/workflows/generate-client.yaml

+      - run: git --no-pager diff
+      - name: Create Pull Request
+        uses: gr2m/create-or-update-pull-request-action@v1


Acutally I kept especially for debugging purposes and local checks, I don't see particular issue in keeping that. Do you see any?

lampajr · 2024-03-20T16:43:18Z

.gitignore

+# Kiota
+**/.kiota.log


lampajr · 2024-03-20T16:45:37Z

Makefile

+# add tools bin directory
+PATH := $(PROJECT_BIN):$(PATH)


yeah makes sense, we can get rid of this and re-introduce if really needed

lampajr · 2024-03-20T16:57:25Z

src/horreum/horreum_client.py

+        if self.__username is not None and self.__password is not None:
+            # Bearer token authentication
+            access_provider = await setup_auth_provider(self.__base_url, self.__username, self.__password)
+            self.auth_provider = BaseBearerTokenAuthenticationProvider(access_provider)
+        else:
+            # Anonymous authentication
+            self.auth_provider = AnonymousAuthenticationProvider()


Yeah that sounds good to me, added check (we can discuss further on the exception message, I am not that good 😄 )

lampajr · 2024-03-20T17:04:01Z

src/horreum/keycloak_access_provider.py

+    def get_allowed_hosts_validator(self) -> AllowedHostsValidator:
+        pass


IIRC pass does not return anything, so it is slighlty different from a return statement.
Anyway to be more expressive I changed to return AllowedHostsValidator(allowed_hosts=[]) which means any host is allowed.

lampajr · 2024-03-21T11:16:47Z

@stalep @webbnh @mfleader I was able to address the exclusion of the generated code (issue #3) as part of this PR as it did not require much effort, this way this PR is much smaller and the final wheel is actually the same as before.

At the moment I'd keep the openapi.yaml committed here.

Moreover I created a publish workflow that aims to automate the publication of the package to Pypi, I still need to figure out if we need to create the package before the first publication or not.

webbnh

Looks good.

I've made a few follow-up comments, and I've called out a few things to consider for your next PR.

At the moment I'd keep the openapi.yaml committed here.

That's fine, but at some point you're probably going to be building clients for more than one branch of Horreum, at which point you'll want to come up with a new plan.

webbnh · 2024-03-21T20:32:36Z

test/horreum_client_test.py

+    try:
+        await new_horreum_client(base_url="http://localhost:8080", password=password)
+        pytest.fail("expect RuntimeError here")
+    except RuntimeError as e:
+        assert str(e) == "providing password without username, have you missed something?"


While this certainly works, Pytest actually provides direct support for this scenario. So, you could code this like

with pytest.raises(RuntimeError) as excinfo: await new_horreum_client(base_url="http://localhost:8080", password=password) assert str(excinfo.value).endswith("providing password without username, have you missed something?")

which is slightly more graceful.

webbnh · 2024-03-21T20:39:15Z

.github/workflows/generate-client.yaml

+      - run: git --no-pager diff
+      - name: Create Pull Request
+        uses: gr2m/create-or-update-pull-request-action@v1


The only issues are that the output might (have) be(en) voluminous. This is not a huge problem (it's not my disk space... 😉), and the workflow web page probably hides it behind an "expansion triangle", but things might go better if we didn't capture it and keep it around (since, if we wanted it, it would be in the pull request).

However, now that you're not including the generated files in the repo, you might want to revisit this. This diff (nor the pull request) will not show the changes to the generated files. I don't know whether that is good or bad, but it removes most of the value from generating the diff here. Of course, without the generated files, the size of this diff is unlikely to be large enough to matter...so maybe it's not worth thinking about anymore. 😁

webbnh · 2024-03-21T20:42:50Z

src/horreum/horreum_client.py

+        if self.__username is not None and self.__password is not None:
+            # Bearer token authentication
+            access_provider = await setup_auth_provider(self.__base_url, self.__username, self.__password)
+            self.auth_provider = BaseBearerTokenAuthenticationProvider(access_provider)
+        else:
+            # Anonymous authentication
+            self.auth_provider = AnonymousAuthenticationProvider()


It looks like you're good enough! 😁

Whether you want to raise a RuntimeError or a ValueError is probably not a big deal. You also have the option of defining your own subclass of Exception (or you can subclass one of those other two), but I would not necessarily go that route unless you're going to have several exceptions that you want your caller to be able to differentiate among (or unless you want your caller to be able to catch your exception without having to catch all RuntimeError's), but you can improve that later when there is an actual need.

webbnh · 2024-03-21T20:47:21Z

src/horreum/keycloak_access_provider.py

+    def get_allowed_hosts_validator(self) -> AllowedHostsValidator:
+        pass


In Python, all functions return something: unless control leaves the function by executing a return (or yield) statement which specifies an explicit value, the function returns a value of None. So, your pass would result in your caller receiving a None value...and that does not conform to your type hint of AllowedHostsValidator.

Now, you could address this by changing the type hint to something like typing.Optional[AllowedHostsValidator] which would permit the None value. Or, you could do as you did and replace the pass with a return with a proper value. (I believe I like your choice better. 😉)

webbnh · 2024-03-21T20:50:52Z

.github/workflows/publish.yaml

+jobs:
+  test:
+    name: ${{ matrix.session }} ${{ matrix.python }}


I don't see where you are defining the matrix. Should that be here, or is it set up in a calling workflow?

webbnh · 2024-03-21T20:55:09Z

src/horreum/raw_client/.gitignore

+# Keep this file versioned
+!__init__.py


Missing newline. 🙂

webbnh · 2024-03-21T20:59:30Z

src/horreum/service/__init__.py

+# from .info_service import get_client_version
+
+__all__ = [
+    "info_service"
+]


What is this file doing for us? Aside from the fact that the import statement is commented out (I'm not a fan of merging modules with commented-out code...), what is the effect of putting a string in the __all__ list?

Used to fetch the client version, to avoid unnecessary complexity I am merging this directly in the horreum_client file

webbnh · 2024-03-21T21:03:09Z

README.md

+Generate source files
+```bash
+make generate
+```
+


Should this change have been made to GET_STARTED.md as well? (Are you still sure you want to have this information in two places?? 🙃)

Yeah I am updating that in a followup PR, as I said in one of the previous comments, this is gonna be replaced by pip install <package> as soon as I can pusblish it on pypi. Therefore this is just a temporary duplication.

lampajr changed the title ~~Python library bootstrap~~ Python library poc Mar 19, 2024

lampajr force-pushed the python_lib branch 2 times, most recently from b360f46 to 1190c71 Compare March 19, 2024 17:06

lampajr marked this pull request as ready for review March 19, 2024 17:06

webbnh reviewed Mar 19, 2024

View reviewed changes

lampajr commented Mar 20, 2024

View reviewed changes

Setup Python client PoC

486c86c

Co-authored-by: Webb Scales <[email protected]>

lampajr force-pushed the python_lib branch from 11be14a to 486c86c Compare March 20, 2024 10:22

lampajr changed the title ~~Python library poc~~ Setup Python client PoC Mar 20, 2024

webbnh mentioned this pull request Mar 20, 2024

Which files should be included in this repo? #3

Closed

webbnh reviewed Mar 20, 2024

View reviewed changes

Applied code review

cc4e91b

lampajr commented Mar 20, 2024

View reviewed changes

lampajr added 3 commits March 21, 2024 12:06

Remove generated code from versioning

4f60d64

Added publish workflow

2993e06

Update README

37c0e92

stalep merged commit 24e6d42 into Hyperfoil:main Mar 21, 2024

lampajr deleted the python_lib branch March 21, 2024 13:41

webbnh reviewed Mar 21, 2024

View reviewed changes

webbnh mentioned this pull request Mar 25, 2024

Setup release process and other improvements #4

Merged

13 tasks

		curl -sLO https://github.com/microsoft/kiota/releases/download/${KIOTA_VERSION}/${OS_NAME}-${OS_ARCH}.zip ;\
		unzip -o linux-x64.zip ;\

		pytest.fail("Unable to fetch Horreum version, is Horreum running in the background?")

		return client

		def get_allowed_hosts_validator(self) -> AllowedHostsValidator:
		pass

		# Kiota
		**/.kiota.log

		[tool.pytest.ini_options]
		asyncio_mode = "auto"

		# Kiota
		**/.kiota.log

Setup Python client PoC #2

Setup Python client PoC #2

Conversation

lampajr commented Mar 19, 2024 • edited Loading

lampajr commented Mar 19, 2024

webbnh left a comment • 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

lampajr Mar 20, 2024 • 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

lampajr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lampajr Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

lampajr commented Mar 20, 2024

mfleader commented Mar 20, 2024

stalep commented Mar 20, 2024

webbnh 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

webbnh Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lampajr left a comment • 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

lampajr commented Mar 21, 2024

webbnh 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

webbnh Mar 21, 2024 • 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

lampajr commented Mar 19, 2024 •

edited

Loading

webbnh left a comment •

edited

Loading

lampajr Mar 20, 2024 •

edited

Loading

lampajr Mar 20, 2024 •

edited

Loading

webbnh Mar 21, 2024 •

edited

Loading

lampajr left a comment •

edited

Loading

webbnh Mar 21, 2024 •

edited

Loading