Skip to content

Conversation

@rustybee42
Copy link
Collaborator

  • Rename --upgrade to --db-upgrade (keeping --upgrade as deprecated alias)
  • Make it an enum, allowing true, false, auto; defaulting to true
  • Add it to the config file (disallowing "true", which would prevent the management from starting)

* Rename --upgrade to --db-upgrade (keeping --upgrade as deprecated
alias)
* Make it an enum, allowing true, false, auto; defaulting to true
* Add it to the config file (disallowing "true", which would prevent the
management from starting)
Comment on lines +11 to +15
# Decides how to upgrade the managements database schema, when required.
# Can be set to "auto" to perform an auto upgrade on startup without having to manually run with
# the --db-upgrade flag. Automatically creates a backup of the existing database file in the same
# directory.
# db-upgrade = "false"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion(blocking): after thinking it over I think it would be more user friendly to just introduce a new boolean db-auto-upgrade parameter. That way we can just keep the --upgrade flag as it is today which avoids needing to update docs, training, etc.

})
.await?;

log::warn!("Database upgraded to version {version}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion(non-blocking):

Suggested change
log::warn!("Database upgraded to version {version}");
log::warn!("Database automatically upgraded to version {version}");

Comment on lines +93 to +95
if schema_check.is_err() {
upgrade_db(&db).await?;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion(non-blocking): This would try to upgrade the DB on any error from the schema_check() including the outside the valid range error where this binary shouldn't try to migrate the on-disk DB.

The migration would never actually be attempted due to the second check in migrate_schema(), but it would try to take a backup then later fail. It would be better just to fail fast.

Ok(())
}

fn upgrade_db(db_file: &Path) -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion(non-blocking): it would be worth mentioning this only is used for the manual upgrade path and for automatic upgrades see the version in lib.rs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants