Which files should be included in this repo?

[webbnh]

From #2 (comment):

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.

Reply:

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 [is] 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)

[...]
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)

The first point of view that I would offer is, is this repo conceived as a stand-alone project or is it intended to be an extension of Hyperfoil/Horreum? As a stand-alone project, arguments around capturing the input to enable reproducibility of the output make reasonable sense; however, if this is just a modular portion of the overall Horreum build, there is no need for that, because the input is tagged as a part of the Horreum build and is therefore easily reproduced, which enables the output to be reproduced.

The idea of needing to modify the generated files after they are generated does support the argument for capturing the results of the build before they are packaged. However, that is a solution to a problem which we do not currently have. And, should that problem arise, there are other solutions which might be be better and which likely have significantly lower costs (and, those costs would be paid only while we actually have the problem and not every time we get a new openapi.yaml file).

As for "offline" environments, as you say, only the openapi.yaml file is needed for this (as well as Kiota, and the build procedures from this repo...), so I think we can support that without having to include any "inputs" or generated files in the repo.

The real problem with including the generated files in the repo is the sheer volume of code and change which is imposed on the repo. PR #2 contains over 18,000 lines of change across almost 200 files, and it is not trivial to determine which of those were generated and which should be considered for code review. Admittedly, some of those files are typical repo set-up stuff (although, one might have wished that much of the setup had been presented in a separate PR...), but I think there are only about a dozen files which actually warrant our attention. So, the other 180 files are just noise; and, any time the OpenAPI spec is changed it will generate more noise. And, this sort of noise interferes with the efficacy of review.

As for adding value to the client, such as authentication/authorization, omitting the generated files from the repo doesn't preclude this. (Although, I believe that both of the other client generators that we tried generated both unauthenticated and authenticated clients and didn't require add-on code.) You can commit added-value modules to the repo, and then the build, after generating the other modules, includes the hand-coded modules with the generated modules when it builds the wheel. The only difference is that the generated modules are "built" and not committed to the repo. (That is, I think that PR #2 would be unchanged except it would omit the src/horreum/raw_client directory from the branch (and, maybe add it to the .gitignore file...).)

So, to be explicit, the repo would contain the boilerplate stuff (issue and PR templates, the README.md and similar files, etc.), the makefile and GH workflow files, and any hand-coded Python files; the build would checkout the repo, pull the openapi.yaml file, generate the contents of src/horreum/raw_client, and then build the wheel; then (aspirationally...) it would install the wheel into a virtual environment, crank up a test Horreum server, and run the functional tests; finally, if all has gone well, it would publish the wheel as a GitHub build artifact or as a GH "release" or it would push it to PyPI. None of this requires the openapi.yaml file or the generated client files to be committed to the repo.

[lampajr]

Hi @webbnh, thanks for opening this issue!

I think that all of your points are valid ones and after discussion with @stalep as well I think we agreed that removing the boilerplate/autogenerated code from the repo is good for all the points you raised.

Regarding the openapi.yaml you are right, as long as it is shipped as part of Horreum we can actually just keep the corresponding Horreum version and extract the spec from that tagged release, on the other hand having it versioned could simplify some other tasks like inspection and debugging. As it is just one file I would keep it at the moment and maybe remove it as part of further improvements.

(Although, I believe that both of the other client generators that we tried generated both unauthenticated and authenticated clients and didn't require add-on code.)

I am not that sure about this especially because the authentication is managed by external systems (in this case Keycloak) so you would always have to instruct the generated client how to retrieve the auth token - anyway, as you said, this doesn't preclude to exclude generated code from the repo.

WDYT @stalep ? I think we agree that removing the auto-generated code is ideal but I would do that in a followup PR to unblock #2 (as it could require some more automation in CI and for local build), any objections @webbnh @stalep ?

[webbnh]

As [openapi.yaml] is just one file I would keep it at the moment and maybe remove it as part of further improvements.

To the extent that this repo is all about building a generated client for and from the results of a given Horreum build, having the openapi.yaml checked in here isn't helpful: for instance, it becomes problematic if you want to build clients for both the 0.12.x and the master branches (unless you check in the respective API files on branches here which mirror those). But, one file is not a big deal, especially in the short term.

(Although, I believe that both of the other client generators that we tried generated both unauthenticated and authenticated clients and didn't require add-on code.)

I am not that sure about this especially because the authentication is managed by external systems (in this case Keycloak) so you would always have to instruct the generated client how to retrieve the auth token

To be honest, I'm not sure, either. I know that the generated clients provided separate client objects for authenticated and anonymous access, and I know that the authenticated client objects "worked" for API calls which did not require authentication (as did the anonymous client objects), but I didn't get far enough to have to work out access to API calls which actually required authentication. I assume that some configuration was required, but I don't know whether the client already understood OIDC or whether the code using it would have to provide it with a Bearer token or the Keycloak equivalent.

this doesn't preclude to exclude generated code from the repo.

🎉 I'm glad that you got that worked out! #2 ended up being "only" 6k lines, of which 4.6k were the openapi.yaml file and another 1k were the poetry.lock file -- ignoring those we landed in a much more appropriate spot. 😁

[webbnh]

I think we can close this issue, now, although I'll leave that to you, @lampajr, and @stalep.

[lampajr]

I working toward further simplication in this #4, e.g., by also removing the openapi.yaml from the committed files as suggested here as well. Thus, let's keep this open till we merge that PR 🚀