Repository navigation
chore: Introduce jsonschema for metadata files validation - #180
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates metadata.yaml validation from an imperative Python implementation to a declarative JSON Schema (jsonschema) approach, updating the validator, its tests, and dependency set accordingly.
Changes:
- Replaced custom metadata validation logic with
jsonschema+ a custom RFC3339/age format checker. - Removed the
semverdependency and enforced semantic-version formatting via JSON Schema regex. - Updated validator tests and test fixtures (including
lastVerifiedvalues) to align with the new validation behavior.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Removes semver from extras and updates the lock accordingly. |
| pyproject.toml | Drops semver from test extras and adds jsonschema. |
| scripts/validate_metadata/validate_metadata.py | Replaces imperative validation with JSON Schema validation and a custom date-time format checker. |
| scripts/validate_metadata/metadata_schema.json | Introduces JSON Schema definition for metadata.yaml structure and constraints. |
| scripts/validate_metadata/validate_metadata_test.py | Updates tests to assert on JSON Schema-driven validation failures and adjusts imports. |
| scripts/validate_metadata/test_data/metadata/valid/valid_metadata.yaml | Updates lastVerified fixture value. |
| scripts/validate_metadata/test_data/metadata/valid/missing_ci_dependency_probe.yaml | Updates lastVerified fixture value. |
| scripts/validate_metadata/test_data/metadata/valid/excluding_tags.yaml | Updates lastVerified fixture value. |
| scripts/validate_metadata/test_data/metadata/valid/excluding_links.yaml | Updates lastVerified fixture value. |
| scripts/validate_metadata/test_data/metadata/valid/excluding_ext_dependencies.yaml | Updates lastVerified fixture value. |
| scripts/validate_metadata/test_data/metadata/valid/excluding_ci.yaml | Updates lastVerified fixture value. |
| scripts/validate_metadata/test_data/metadata/valid/excluding_ci_dependency_probe.yaml | Updates lastVerified fixture value. |
| scripts/validate_metadata/test_data/metadata/valid/custom_links_category.yaml | Updates lastVerified fixture value. |
| scripts/validate_metadata/test_data/metadata/invalid/invalid_tag_array_type.yaml | Updates lastVerified fixture value. |
| scripts/validate_metadata/test_data/metadata/invalid/invalid_dependency_semantic_versioning.yaml | Updates lastVerified fixture value. |
| scripts/validate_metadata/test_data/metadata/invalid/invalid_dependencies_type.yaml | Updates lastVerified fixture value. |
| scripts/validate_metadata/test_data/metadata/invalid/invalid_dependencies_category.yaml | Updates lastVerified fixture value. |
| scripts/validate_metadata/test_data/metadata/invalid/invalid_ci.yaml | Updates lastVerified fixture value. |
| scripts/validate_metadata/test_data/metadata/invalid/invalid_ci_dependency_probe.yaml | Updates lastVerified fixture value. |
| scripts/validate_metadata/test_data/metadata/invalid/invalid_ci_category.yaml | Updates lastVerified fixture value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/ok-to-test |
| import validate_metadata | ||
| from validate_metadata import ValidationError |
There was a problem hiding this comment.
Well, this is true. However this is exactly how it was before. Again, my intention was to introduce minimal set of changes.
@hbelmiro should we align this with other directories under scripts/*?
There was a problem hiding this comment.
Let's keep this PR focused. I'll send a PR to fix that and then you can rebase.
|
@filip-komarzyniec can you please rebase? No CI issues are expected now. |
e39f6ac to
9dbabea
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
pyproject.toml:44
- The bare
jsonschemadependency does not install the optional RFC 3339 format checker, yetvalidate_metadata.py:21indexesFormatChecker.checkers["date-time"]during import. The regenerated lockfile confirms thatrfc3339-validatoris absent, so a clean CI environment will raiseKeyError: 'date-time'before validation can run. Depend onjsonschema[format-nongpl](or explicitly onrfc3339-validator) and regenerate the lockfile.
"jsonschema"
scripts/validate_metadata/validate_metadata.py:43
- This still rejects valid RFC 3339 timestamps with a lowercase
z. The built-in checker accepts them by normalizing case, butdatetime.fromisoformat(instance)receives the original value and raisesValueError, so the PR's RFC-3339 validation remains inconsistent. Normalize the timestamp before parsing (and add the lowercase-zregression case).
if (now - datetime.fromisoformat(instance)).days >= 365:
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
scripts/validate_metadata/validate_metadata.py:43
- The built-in RFC 3339 checker accepts a lowercase
zUTC suffix, butdatetime.fromisoformat()does not. Consequently, a valid value such as2026-07-15T00:00:00zpasses the format check and then raises inside this freshness check. Normalize the suffix before parsing.
if (now - datetime.fromisoformat(instance)).days >= 365:
| return False | ||
|
|
||
| now = datetime.now(tz=timezone.utc) | ||
| if (now - datetime.fromisoformat(instance)).days >= 365: |
There was a problem hiding this comment.
datetime.fromisoformat() rejects lowercase z as a UTC designator even on Python 3.13 (confirmed locally). RFC 3339 treats z and Z as equivalent, and the rfc3339-validator package (backing jsonschema's date-time format checker) accepts both.
A valid metadata value like 2026-07-15T00:00:00z passes the format check but then raises ValueError in the staleness check, producing a misleading error.
The previous thread was resolved claiming a fix, but the current code still uses datetime.fromisoformat(instance) without normalization.
Suggested fix: Normalize the input before parsing:
if (now - datetime.fromisoformat(instance.upper())).days >= 365:There was a problem hiding this comment.
Right, this is a valid concern.
At the very beginning I was quite sure the RFC3339 is a subset of ISO8601. It turns out this is not the exact case: https://ijmacd.github.io/rfc3339-iso8601/
The z vs. Z is not the only case where these format differ. However, the RFC3339 that are not valid ISO8601 are quite specific cases (and not that many) so merging the PR in its current form brings (IMO) negligible risk. I wanted to open a separate issue for this after merge.
Because you're bringing this up, I think these are our available solutions:
- change the datetime restriction format to ISO8601. The RFC3339 previously was not properly enforced either (even worse), however according to the docs and comments it was meant to be RFC3339. That is why I'm trying to keep it this way
- pull in additional (test extras) libraries for rfc3339 parsing like
dateutilorpyRFC3339, - fail format checking once the provided instance is a valid RFC339 and invalid ISO8601 with appropriate error message.
- do custom data parsing covering chosen cases where rfc3339 differs from iso8601. The one you proposed above and a couple of different tweaks like:
- normalization of
_and spaces separating datetime components - shortening possible >6 fraction seconds digits
- handling
-00:00offset
- normalization of
What do you think would be our best bet? I'd recommend reflecting on why the restriction to RFC3339 was made in the first place. And if it's still valid --- pull one of the libraries for working with this format. Custom cleaning will always have some logic holes in it...
There was a problem hiding this comment.
What about enforcing the exact format everyone already uses? Every lastVerified in the repo is YYYY-MM-DDTHH:MM:SSZ. We could add a pattern to the schema:
"lastVerified": {
"type": "string",
"format": "date-time",
"pattern": "^\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}Z$"
}That way format: date-time still triggers the freshness check, and the pattern eliminates the RFC 3339 vs ISO 8601 ambiguity without pulling in extra libraries. Thoughts?
There was a problem hiding this comment.
Indeed sounds like the perfect middle ground, great idea!
However the JsonSchema runs validators in the random order (acc. to jsonschema 2020 specification and this comment: python-jsonschema/jsonschema#1519 (comment))
That's why, I believe, instead of specifying the exact pattern under pattern field, let's move it to the function performing date-time check (check_date_time function).
We can also add to the field's description that the format is fixed to one of the RFC3339-compliant ones (and even give a specific example).
Having the pattern check in both pattern field and check_date_time function could lead to different errors triggered by the same event which is definitely unwanted.
Do you agree with this solution?
There was a problem hiding this comment.
Yeah. Definitely a single point of validation.
…pipeline) Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com>
…se jsonschema; deleted semver dependency Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com>
…nium forward so that test cases do not fail Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com>
Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com>
…oading; fixed typos Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com>
Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com>
… millennium forward so that test cases do not fail Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com>
…n` fields; related tests changes + add mistakenly deleted test case Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com>
…chema loading; fixed typos Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com>
… mocked millennium forward so that test cases do not fail Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com>
…rated lockfile Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com>
…; updated tests, lockfile and metadata schema Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
2b403be to
802793b
Compare
| """ | ||
| # jsonschema runs validators in random order as per specification: | ||
| # https://github.com/python-jsonschema/jsonschema/issues/1519#issuecomment-4980606159 | ||
| if not (isinstance(instance, str) or re.match(r"\d{4}(-\d{2}){2}T(\d{2}:){2}\d{2}Z$", instance)): |
There was a problem hiding this comment.
| if not (isinstance(instance, str) or re.match(r"\d{4}(-\d{2}){2}T(\d{2}:){2}\d{2}Z$", instance)): | |
| if not (isinstance(instance, str) and re.match(r"\d{4}(-\d{2}){2}T(\d{2}:){2}\d{2}Z$", instance)): |
There was a problem hiding this comment.
ahhh, rookie mistake 🥲 Thank you for thorough validation!
…rcement; updated tests, lockfile and metadata schema Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hbelmiro The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* introduced jsonschema for metadata.yaml file (req for each compoennt/pipeline) Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> * components/pipelines metadata.yaml validation scripts refactored to use jsonschema; deleted semver dependency Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> * test metadata.yaml files' required `lastVerified` field mocked millennium forward so that test cases do not fail Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> * updated project's lockfile Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> * introduced changes pointed out in PR review, mostly: changed schema loading; fixed typos Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> * fixup! updated project's lockfile Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> * fixup! test metadata.yaml files' required `lastVerified` field mocked millennium forward so that test cases do not fail Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> * add back `experimental` stability value; rm `managed` and `description` fields; related tests changes + add mistakenly deleted test case Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> * fixup! introduced changes pointed out in PR review, mostly: changed schema loading; fixed typos Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> * fixup! fixup! test metadata.yaml files' required `lastVerified` field mocked millennium forward so that test cases do not fail Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> * added rfc3339-validator test dependecy for timestamp checking; regenerated lockfile Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> * removed rfc3339 validator due to stricter datetime format enforcement; updated tests, lockfile and metadata schema Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> * fixup! removed rfc3339 validator due to stricter datetime format enforcement; updated tests, lockfile and metadata schema Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> --------- Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com>
…eflow#180) Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Description of your changes:
Checklist:
Changes
jsonschemafor metadata.yaml file instead of a complicated, imperative script verifying metadata.yaml contentdeleted duplicated test cases (validate_metadata_test file)semverdependencyjsonschemadependency (it was already pulled by multiple libraries)lastVerifiedfield of multiple metadata.yaml data files used for testing to a millenium forward. These data files are specifically designed to test other scenarios than any connected withlastVerifiedfield. Setting it to be valid for further 1000 years will avoid unwanted problems with tests suitescripts/validate_metadata/validate_metadata_test.py-- deleted relative imports in favour of absolute ones. Relative imports are too frequently problematic as they behave differently based on the way of executing the program.Resolves: #184
Pre-Submission Checklist
Learn more about the pull request title convention used in this repository.
Additional Checklist Items for New or Updated Components/Pipelines
metadata.yamlincludes freshlastVerifiedtimestampare present and complete
snake_casenaming convention