feat(grzctl, grz-db): Metadata schema version check via version.json#624
feat(grzctl, grz-db): Metadata schema version check via version.json#624virag-compbio wants to merge 2 commits into
Conversation
|
btw, I'd try to avoid the term "S3 policy", because that can easily be confused with "S3 bucket policy" for example; the policy we're talking about here is not a property of / does not pertain to S3, it just happens to be stored in S3. |
virag-compbio
changed the title
All the tests pass now and the I have removed any references to "S3 policy" - it was indeed misleading, thanks for the comment. |
virag-compbio
requested review from
Hoeze and
tedil
and removed request for
Hoeze and
tedil
virag-compbio
force-pushed
the
feature/metadata-version-check-utility
branch
from
2e90f29 to
3892730
Compare
virag-compbio
force-pushed
the
feature/metadata-version-check-utility
branch
from
3892730 to
cee5d9f
Compare
| threads=threads, | ||
| ) | ||
|
|
||
| config = ValidateConfig.model_validate(configuration) |
There was a problem hiding this comment.
there already is an instance of this available from line 43
|
|
||
| class ValidateConfig(IdentifiersConfigModel): | ||
| pass | ||
| s3: Annotated[S3Options | None, Field(default=None)] = None |
There was a problem hiding this comment.
... = None already is the default, no point in also specifying Field(default=None); and in that case, Annotated is also not needed, so this should do:
| s3: Annotated[S3Options | None, Field(default=None)] = None | |
| s3: S3Options | None = None |
There was a problem hiding this comment.
But a different question: Should this whole thing even be part of validate or should this already be checked during download (where we naturally already have the S3 config and which does do the basic schema validation for the metadata anyway)?
| logger.info(f"grz-cli {current_version} is within the supported and tested range.") | ||
|
|
||
|
|
||
| def check_metadata_version_and_exit_if_needed( |
There was a problem hiding this comment.
please keep the code DRY; this is basically 1:1 the same as check_version_and_exit_if_needed, should be possible to generalize into a shared function just with an extra parameter or so.
| @pytest.mark.parametrize("grz_check_mmap", ["--mmap", "--no-mmap"]) | ||
| def test_validate_submission( | ||
| temp_identifiers_config_file_path, | ||
| temp_s3_config_file_path, # <-- add this fixture |
There was a problem hiding this comment.
please remove stray AI progress comments ^^
| "--config-file", | ||
| temp_identifiers_config_file_path, | ||
| "--config-file", | ||
| temp_s3_config_file_path, # <-- provides S3 config |
2 participants
Metadata schema version check via version.json stored in the LE buckets
Resolves #557
Problem: Metadata schema version acceptance was hardcoded in get_accepted_versions() in grz-pydantic-models, requiring code updates to change which schema versions are accepted.
Fix: Extended the existing version.json (used for grz-cli version checks) to support metadata schema version enforcement:
i) Added metadata_version field to VersionFile model in grz-common
ii) Added check_metadata_version_and_exit_if_needed() function in grz-cli that fetches version.json from S3 and compares the submitted metadata's schema version against the policy
iii) Made s3 optional in ValidateConfig to support offline validation (check skipped when no S3 config provided)
iv) Called the metadata version check in the validate command after parsing the submission
v) Extended remote_bucket_with_version fixture to include metadata_version in test version.json
vi) Added integration test for metadata version check with S3 config
vii) Mocked the version check in existing validate tests that don't use S3 config
The metadata schema version is extracted from the submitted metadata.json via get_schema_version() on the GrzSubmissionMetadata model.