Add upgrade DB to CLI #319
Conversation
doaa-altarawy
added
Database
qcfractal/cli/qcfractal_server.py
Outdated
| @@ -226,7 +239,7 @@ def server_start(args, config): | |||
| logfile = str(config.base_path / config.fractal.logfile) | |||
|
|
|||
| print("\n>>> Checking the PostgreSQL connection...") | |||
| database_name = args.get("database_name", None) or config.database.default_database | |||
| database_name = config.database.database_name | |||
There was a problem hiding this comment.
Do we need to create this alias, is it possible just to use it directly below?
qcfractal/config.py
Outdated
| @@ -100,6 +102,9 @@ class Config(SettingsCommonConfig): | |||
|
|
|||
| class FractalConfig(ConfigSettings): | |||
|
|
|||
| # class variable, not in the pydantic model | |||
| defaults_file_path: ClassVar[str] = os.path.expanduser("~/.qca/qcfractal_defaults.yaml") | |||
There was a problem hiding this comment.
The other thing to do here is use a _defaults_file_path. This will tell pydantic to automatically consider it a class var.
| if Path(FractalConfig.defaults_file_path).exists(): | ||
| with open(FractalConfig.defaults_file_path, "r") as handle: | ||
| kwargs['base_folder'] = yaml.load(handle.read(), | ||
| Loader=yaml.FullLoader)['default_base_folder'] |
qcfractal/postgres_harness.py
Outdated
|
|
||
| if ret['retcode'] != 0: | ||
| raise ValueError("\nFailed to Upgrade the database, make sure to init the database first before being able to upgrade it.\n") | ||
| print(ret) |
There was a problem hiding this comment.
This print statement will not be executed.
|
|
||
| cmd = [shutil.which('alembic'), | ||
| '-c', self._alembic_ini, | ||
| '-x', 'uri='+self.config.database_uri(), |
There was a problem hiding this comment.
Could you use a f-string here to match other areas?
|
|
||
| ret = self._run([shutil.which('alembic'), | ||
| '-c', self._alembic_ini, | ||
| '-x', 'uri='+self.config.database_uri(), |
| @@ -315,7 +315,6 @@ def start(self, timeout: int = 5) -> None: | |||
| self._qcfractal_proc = _background_process([ | |||
| shutil.which("qcfractal-server"), | |||
| "start", | |||
| f"--database-name={self._dbname}", | |||
There was a problem hiding this comment.
This will create some restart issues. I think we will need to init a new temporary folder and qcfractal-init into that every time.
|
This pull request introduces 5 alerts when merging 01111a8 into e96ff16 - view on LGTM.com new alerts:
|
|
This pull request introduces 3 alerts when merging 2549ae3 into ea58476 - view on LGTM.com new alerts:
|
|
Yes, ready to merge. |
Description
Added alembic migrations to the CLI using the command
qcfractal-server upgrade.TODO:
[x] Add testing
Status