asserting that ingest exports valid provenance metadata#509
asserting that ingest exports valid provenance metadata#509rolando-ebi wants to merge 3 commits intomasterfrom
Conversation
| - apt-get -y install jq | ||
| - pip install -r requirements.txt | ||
| - export DEPLOYMENT_ENV=$CI_COMMIT_REF_NAME | ||
| - export DEPLOYMENT_ENV=integration |
There was a problem hiding this comment.
$CI_COMMIT_REF_NAME resolves to the deployment being tested in the scheduled dcp-wide integration tests, e.g. integration, staging, and prod. Shouldn't this change be reverted to continue testing in the other environments?
There was a problem hiding this comment.
Yes, this is a temporary change to just get the test running and passing in gitlab. It should be reverted once the changes are ready to merge
There was a problem hiding this comment.
An intermittent issue un-related to the changes caused the previous test failure for this branch. Will re-run and post a link to the completed pipeline
|
What is the status of this PR? |
|
Hey @rolando-ebi , somehow I wasn't aware of this ticket getting assigned to me. Do you remember what this was about? |
maniarathi
left a comment
maniarathi
left a comment
There was a problem hiding this comment.
Apologies for the long delay on this review! Better late than never right? :)
| only: | ||
| - integration | ||
| - staging | ||
| - validate-schema-versions |
There was a problem hiding this comment.
I think I'm a little confused why validate-schema-versions is an environment listed here and likewise below?
| raise RuntimeError(f'Expected {self.expected_bundle_count} primary bundles, but only ' | ||
| f'got {primary_bundles_count}') | ||
|
|
||
| def assert_valid_schema_versions_in_provenance(self): |
There was a problem hiding this comment.
Could we please move the assert function down to join the group of other assert functions below? (Roughly line 387).
There was a problem hiding this comment.
Perhaps also write a small docstring for this function please so that folks who aren't familiar with the schema versions in the provenance can understand what is being validated here?
| for metadata_file in metadata_files: | ||
| schema_url = metadata_file["describedBy"] | ||
| schema = requests.get(schema_url).json() | ||
| validate(metadata_file, schema=schema) |
There was a problem hiding this comment.
This validate function won't actually check to make sure that schema and the major and minor versions of the metadata file will match because in the schema, the pattern just says something like "^(http|https)://schema.(.?)humancellatlas.org/type/biomaterial/(([0-9]{1,}.[0-9]{1,}.[0-9]{1,})|([a-zA-Z]?))/donor_organism".
At least this is what I think. Have you verified to make sure this test fails if there is a mismatch?
| schema = requests.get(schema_url).json() | ||
| validate(metadata_file, schema=schema) | ||
|
|
||
|
|
There was a problem hiding this comment.
Delete extra line break here.
| from ..data_store_agent import DataStoreAgent | ||
| from ..dataset_fixture import DatasetFixture | ||
| from ..dataset_runner import DatasetRunner | ||
| from tests.utils import Progress, Timeout |
There was a problem hiding this comment.
I think these changes are not needed once you are done testing in your own environment. Maybe remove them from the PR and keep these changes local/untracked?
4 participants
No description provided.