cloud: schema feature added #394
Conversation
There was a problem hiding this comment.
This is a good start, now you need to write the code to accept (and validate) the version number from the command line, pass it around where it's needed, and apply it to other database types as well.
Regarding testing, I've created a Google Cloud project for you to test your changes. It's called kcidb-abhishek and you should've received an invitation to join it via email. Next you need to install the Google Cloud SDK, create a service account with administrator permissions, and create and download a key for it. Then you could export its location via the GOOGLE_APPLICATION_CREDENTIALS environment variable, and after that you would be able to run the cloud tool to make a test deployment. Try with an unmodified cloud tool first. The command to deploy to your project would look something like ./cloud deploy kcidb-abhishek "" 1 -v --smtp-mocked --test --heavy-asserts. Use the Administrator Guide as a reference.
|
@spbnick Need reviews |
There was a problem hiding this comment.
This is starting to look good. Thank you, @octonawish-akcodes. I left a few comments inline.
However, I just realized that we can't just pass the same schema version to each database type (BigQuery/PostgreSQL/sqlite), as they could be totally different, because the schema development for different databases has different needs and moves at different speeds.
However, we can specify the I/O schema version, which the databases should support instead, and that has to be the same. The kcidb.db.Client class supports that, but none of the database-managing command-line tools do. So we would need a commit before this one adding support for that.
We could make this work if we e.g. allow the --schema options for these tools to accept a string in the shape of io:<major>.<minor> for the I/O schemas. While we're at it, we could also allow it to accept the string latest, meaning the latest database schema version, and that would help you get rid of those "specified/unspecified schema version" branches you're adding to database initialization functions in cloud.
cloud
Outdated
| if [[ "$arg" == --schema=* ]]; then | ||
| params_value["schema"]="${arg#*=}" | ||
| continue | ||
| fi |
There was a problem hiding this comment.
You don't need to change anything here. Whichever options this functions would support parsing is determined entirely by its arguments. It's enough to just add schema to its arguments in whichever functions need to accept the option.
cloud
Outdated
| @@ -2164,7 +2186,7 @@ function execute() { | |||
| getopt_longopts+=",smtp-mocked,test" | |||
| if [[ $command == @(deploy|withdraw) ]]; then | |||
| getopt_shortopts+="vs:" | |||
| getopt_longopts+=",verbose,sections:,submitter:" | |||
| getopt_longopts+=",verbose,sections:,submitter:,schema:" | |||
There was a problem hiding this comment.
This adds support for the option to both deploy and withdraw commands, and we only need to add it to the deploy command.
There was a problem hiding this comment.
But like in the withdrawing stage doesnt it need the schema values?
There was a problem hiding this comment.
The "withdraw" and all other stages would be able to retrieve the schema version from the databases themselves, so only "deploy" needs it to know which schema to initialize the databases to.
cloud
Outdated
| @@ -2218,6 +2241,7 @@ function execute() { | |||
| --heavy-asserts) heavy_asserts="true"; shift;; | |||
| --mute-updates) updated_publish="false"; shift;; | |||
| --test) test="true"; shift;; | |||
| --schema) schema_version="$2"; shift 2;; | |||
There was a problem hiding this comment.
You're declaring one variable, but assigning to another. Please fix either one or the other.
Also, I think it's best to use the same name both for the option and the variable name. Considering that the word "schema" is not being used anywhere in cloud before your change, we can just use it for both the option and the variable names (and not --schema-version/schema_version).
|
Tried to implement I/O versions to the db command-line tools, am I on the right track? Need reviews. |
There was a problem hiding this comment.
This is a good start.
Note that only two tools use the --schema option for now: kcidb-db-init and kcidb-db-upgrade, no other tool needs it (including kcidb-db-query).
Instead of copying the option-parsing code around, create another "argparse type" function, similar to kcidb.misc.version() and schema_version() inside kcidb.misc.argparse_schema_add_args(). Then both the tools would be able to use it. The function should produce a value which would be accepted by both kcidb.db.Client.init() and kcidb.db.Client.upgrade().
Actually, this might need an extraction of a method looking up a database schema corresponding to an I/O schema, from both init() and upgrade(), in kcidb.db.Client, and then use of that method in both of them. This would make it possible to use that new method to check and report an error whenever the resulting database version is older than the current one, in kcidb-db-upgrade (instead of just failing an assertion in init() and upgrade()).
Support for schema version for initializing the db with versions specified.
This PR will closes #372
@spbnick need reviews
Am I on the right track? I would appreciate some guidance. Also how can I test it.