U/jrbogart/in place mod #180
Conversation
scripts/create_registry_schema.py
Outdated
| config_file=args.config, | ||
| schema=schema, | ||
| entry_mode=entry_mode, | ||
| query_mode=query_mode) |
There was a problem hiding this comment.
entry_mode and query_mode are not needed when schema is set. I think this is was always true, however this is now explicitly set in my latest PR, where I try to make sure connecting to a schema vs namespace sets all the right things.
scripts/create_registry_schema.py
Outdated
| query_mode=query_mode) | ||
| print(f"Database dialect is '{db_connection.dialect}'") | ||
| if args.sqlite != (db_connection.dialect == "sqlite"): | ||
| raise ValueError("config and sqlite argument disagree") |
There was a problem hiding this comment.
If this check can already be done using the dialect, then is the --sqlite flag needed? If this was only for the entry_mode and query_mode then its no longer needed.
There was a problem hiding this comment.
Yes, it was for entry_mode and query_mode. It's true that, once DbConnection determines the dialect is sqlite, it can choose to ignore the modes and the namespace. And if the schema argument is used, DbConnection can set the modes to match that schema. But for sqlite there is no schema, so the schema argument to DbConnection can't be used. Although it might "just work" for the sqlite case, it's very unclear what's going on. At the very least there needs to be more explanation in the docstring for what happens in all these cases.
.github/workflows/ci.yml
Outdated
| @@ -219,7 +219,7 @@ jobs: | |||
| # Create schema | |||
| - name: Create data registry default schema | |||
| run: | | |||
| python scripts/create_registry_schema.py --config $DATAREG_CONFIG | |||
| python scripts/create_registry_schema.py --config $DATAREG_CONFIG --sqlite | |||
There was a problem hiding this comment.
See comment in create_registry_schema
| # For location_type external or metadata_only, relative_path | ||
| # must be none | ||
| if kwargs_dict["location_type"] in ["external", "meta_only"]: | ||
| kwargs_dict["relative_path"] = None |
There was a problem hiding this comment.
After _validate_register_inputs, back in the register function, there is the relative_path from name auto generation. As the relative_path gets set to None before then, is the auto generation then undoing this? (line 454 of register.py)
There was a problem hiding this comment.
Indeed. That's what should be changed rather than _validate_register_inputs.
Will fix.
docs/source/for_maintainers.rst
Outdated
| @@ -0,0 +1,62 @@ | |||
| .. _maintenance | |||
There was a problem hiding this comment.
Need to add this page to the index.rst toctree for it to appear in the docs
| import os | ||
| import argparse | ||
| from sqlalchemy import text | ||
| from dataregistry.db_basic import DbConnection |
There was a problem hiding this comment.
Should we list on the docs page a change log history of these kind of changes?
There was a problem hiding this comment.
Perhaps. I didn't attempt to get this ready for a release (updating version, adding anything to CHANGELOG.md). With more than one person working in the repo I suspect it might be easier to manage if upgrades involving version changes are separated into two PRs:
- code updates (maybe also change version to a value like 1.2.3rc1 so it's clear it's not a release?)
- anything having specifically to do with release, which maybe is just setting version and adding to CHANGELOG.md
…th when creating a new schema or modifying one in place
JoanneBogart
force-pushed
the
u/jrbogart/in_place_mod
branch
from
9898fcf to
2cafd74
Compare
Add a script for removal of "not null" constraint on dataset.relative_path. This entailed other supporting updates:
--sqliteoption when invoking create_registry_schema