-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Minimal CLI for uploading functionality #55
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Map out the initial commands and (almost) finish upload and config.
Add testing dependencies (pytest). Add a special pylint run for tests and update workflows accordingly. Start the first test file for the `entities-service upload` CLI command. Also, fix filesuffix error for a workflow.
Prefer CLI-specific .env before a service-specific .env, and otherwise fallback to CONFIG defaults.
These are currently only used in 'config show' to return the output in the given format, but are made as global options to be available whenever an output of that kind is desired.
Change --use-service-dotenv/--use-cli-dotenv to a simple --dotenv-path instead, to point to a .env file to use.
Validate via the pydantic SOFT5 and SOFT7 models instead of trying to instantiate entities in DLite. This greatly simplifies things and automatically implements Python 3.12 support for the CLI. Add helpful READMEs in the static folders for valid and invalid entities to describe them. This was done especially to showcase the validation errors pydantic throws for the invalid entity Person.json and to announce the Cat.json valid entity as a SOFT5 entity (the others are SOFT7 entities).
The new logic for the upload CLI command is: Check if the incoming entity's URI already exists in the backend. If no, upload. Otherwise, check if the incoming entity is equal in every way to the existing entity in the backend. If yes, skip and continue. Otherwise, have the user confirm if they want to upload the incoming entity with a new version. If no, skip and continue. If yes, ask the user to enter the new version, while providing a reasonable default value. If user aborts, skip and continue. Otherwise, update the incoming entity with the new version and upload it. We also add a new entity with a "ref" property type, since we need to make sure special keys can be uploaded to the backend (Mongo does not support keys starting with $).
Handle the $ character in the upload logic instead, before uploading to the backend.
Add extra final summary information about skipped entities, if any.
Extend tests - this was added due to testing.
Especially add multiple invalid entities to ensure the SOFT5 and SOFT7 validations are caught.
This was referenced Dec 20, 2023
ajeklund
reviewed
Jan 9, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice! Added a few questions/suggestions.
CLI message clarifications for users Co-authored-by: Anders Eklund <[email protected]>
Same as other code review commit Co-authored-by: Anders Eklund <[email protected]>
ajeklund
approved these changes
Jan 10, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the nice work!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #12
This is a replacement for #17, which only implements the
upload
andconfig
CLI commands.To do:
upload
config
global_settings.py
The CLI is based on Typer, which is made by the same author of the popular FastAPI package for building REST APIs. FastAPI is what is used for OTEAPI Services and many other of our REST API repositories.
The way this CLI is built is similar to FastAPI and therefore makes is easy to build, maintain and improve upon.
The CLI is made up of the following commands:
upload
config set
config unset
config unset-all
config show
With some global options to for example show the configuration (for the DLite Entities Service) in a specific format (for example JSON or YAML). As well as a choice between two configuration file locations for the two different modules that now exist in the repository: the REST API service and the CLI.
All commands are tested with unit tests and there's complete code coverage for all new changes.
Edit: I've had a thought about the implementation of this (especially with the SINTEF firewall and internal port blocking in mind). Instead of connecting to the database directly in the CLI we should go through a service endpoint to upload a (verified) data model, and the service will then handle all communication with the database.
This ensures:
For now, I'd keep this PR as is, though, and then implement this update in a separate PR.
Edit: Another issue that should be raised and fixed in another PR is the duplication of the SOFT5/7 pydantic models. Either a base model should be created or the SOFT7 model should inherit from SOFT5 implenting it's own "updates".