feat: add script to generate OAI models #27
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new script, File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @aaravm - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Some more general comments:
- Consider creating section headers for better structuring, e.g., "Parameter definitions", "Function definitions", "Main"
- The script sometimes mixes up definitions, setup, business logic and cleanup code. Look at each function and consider reordering so that each function starts with parameter definitions, followed by additional setup code (e.g., creating or deleting files and dirs), business logic (e.g., calling the OpenAPI generator to create the models) and, finally, cleanup code (copy results, remove temporary files and other artifacts).
- Not a huge fan of pulling the OpenAPI generator JAR to
~/bin/openapitools/. For one, the user may already have it in a different location. Might be better to make this configurable. Also consider writing a separate install script here (would need to run only once before executing the script several times as discussed elsewhere in the comments).
|
@uniqueg I have added all of your suggestions. Please mention if you want me to change something else too |
There was a problem hiding this comment.
I'm not quite sure what's the relevance/utility of this script. We cannot generate the APIs on the fly, because we know that the results are generally not good enough and need manual edits. So that means that we will commit the models. Which in turn means that after they have been committed, the script becomes useless.
IMO, it would make more sense if it was parameterized, i.e., if the arguments (which models to generate and where to put them) would be passed via the CLI. That way it could be used to get a first draft of any new specs we add - without redoing what we already have.
But still it would be just a wrapper script, essentially writing several lines of code just to replace what is probably just a single line of code.
So instead of all this, I think just documenting the necessary steps, together with concrete code examples, somewhere in a markdown file (maybe in docs/autogenerate_OAI_models.md, linked to README.md) would be good enough for us and others to contribute models for new APIs (or API versions) in the future, no?
If you disagree, at the very least, this PR should contain:
./script.sh- Documentation on how/when to run this
And it probably would make sense to put the scripts in some subdirectory (e.g., utils/) rather than just dumping them in the repository root directory.
Please tell me what you think, @aaravm and @pavelnikonorov. I guess not merging this for now doesn't block anything, because as far as I can tell, this is not code that is called when building or executing the project and the artifacts the script produces are merged in other PRs (please correct me if I'm wrong).
@uniqueg, Thanks for the review! |
…t/models-autogeneration
…aai/ga4gh-sdk into feat/models-autogeneration
uniqueg
changed the title
…t/models-autogeneration
…t/models-autogeneration
|
@pavelnikonorov @uniqueg, you both had pretty valid concerns about the installation of OpenAPI in the script, so I have moved it outside the script, and provided documentation to see alternate ways to install OpenAPI. |
Yes, please create an issue for using the Docker image as an alternative to manual installation. |
| 1. OpenAPI Generator CLI: Make sure the directory `~/bin/openapitools` is empty, as OpenAPI is being installed in this script, which might conflict with your existing directory. Then run the following script to install OpenAPI generator CLI. | ||
| ```sh | ||
| sudo chmod +x ./utils/script.sh | ||
| mkdir -p ~/bin/openapitools | ||
| curl https://raw.githubusercontent.com/OpenAPITools/openapi-generator/master/bin/utils/openapi-generator-cli.sh > ~/bin/openapitools/openapi-generator-cli | ||
| chmod u+x ~/bin/openapitools/openapi-generator-cli | ||
| export PATH=$PATH:~/bin/openapitools/ | ||
| ``` |
There was a problem hiding this comment.
Not sure why you don't wanna use npm? https://openapi-generator.tech/docs/installation/. No need to manually deal with setting the dir, downloading, making executable etc.
There was a problem hiding this comment.
In that case, you would of course need to install JDK first (which anyway makes sense).
Introduced a script to automate the generation of Rust models from OpenAPI specifications, ensuring the necessary structs are built for the main functions.
Summary by Sourcery
This pull request adds a new script, build-models.sh, which automates the generation of Rust models from OpenAPI specifications. The script ensures the necessary structs are built for the main functions by downloading the OpenAPI Generator JAR and processing the OpenAPI specifications.