Check manifest fields for type errors.

[dblock]

Signed-off-by: dblock dblock@dblock.org

Description

• Add type annotations and type checking to manifest fields, catching ref values that are floats instead of strings.
• Bring in dataclass type annotation checker from https://gist.github.com/svermeulen/8a6ad727113a6d07f78332152c1d33b2, this should really be PRed upstream to Type checking for dataclasses agronholm/typeguard#161, I might take it up.
• Inherit TestManifest from Manifest, enabling from_file and from_path, add tests.

Issues Resolved

Closes #470.

Check List

• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[owaiskazi19]

Nit: Not sure but can we have the name of the file as typechecker as typechecked is an internal keyword?

[dblock]

Renamed to match the class name, dataclass_typechecked.

[owaiskazi19]

New line here

[dblock]

Added.

[owaiskazi19]

Few should have's

[peternied]

This seems like a large/complex amount of work when all the types in the build manifest are strings. Is there a usecase where we need to support numeric values?

Could we do something simpler where we automatically convert all values to strings?

[dblock]

This seems like a large/complex amount of work when all the types in the build manifest are strings. Is there a usecase where we need to support numeric values?

Could we do something simpler where we automatically convert all values to strings?

I see the following options.

1. This change.
2. We could not merge this and str() the ref on a git repo (or str all the non-dict fields). This would be a hack to work around the fact that 1.1 is of type float and not a string, and there's no guarantee that a branch name such as 1.01 doesn't suddenly convert to a string 1.099999 or something like that in the future, which will be hard to debug.
3. We could check explicitly that this one field is an str instead of adding a typechecker. Feels not very future-proof. Plus the check as is also catches issues such as not specifying a dictionary where it should be one - it's an actual validator for manifest schema.

More importantly, agronholm/typeguard#161 is opened to add the dataclass type checking code to the typeguard library, so the complex code will be then removed from here. There's really not much going on other than that :)

[dblock]

@peternied LMK what you think - this does fix a bug and prevents future bugs.

[codecov-commenter]

Codecov Report

Merging #485 (0972e82) into main (3a6dbae) will increase coverage by 1.93%.
The diff coverage is 87.73%.

@@            Coverage Diff             @@
##             main     #485      +/-   ##
==========================================
+ Coverage   69.31%   71.25%   +1.93%     
==========================================
  Files          58       59       +1     
  Lines        1509     1607      +98     
==========================================
+ Hits         1046     1145      +99     
+ Misses        463      462       -1     

Impacted Files	Coverage Δ
...ow/src/ci_workflow/ci_check_gradle_dependencies.py	100.00% <ø> (ø)
...src/manifests_workflow/component_opensearch_min.py	100.00% <ø> (ø)
...le-workflow/src/manifests/dataclass_typechecked.py	79.03% <79.03%> (ø)
bundle-workflow/src/manifests/build_manifest.py	100.00% <100.00%> (ø)
bundle-workflow/src/manifests/bundle_manifest.py	100.00% <100.00%> (ø)
bundle-workflow/src/manifests/input_manifest.py	100.00% <100.00%> (ø)
bundle-workflow/src/manifests/manifest.py	96.66% <100.00%> (+0.11%)	⬆️
bundle-workflow/src/manifests/test_manifest.py	100.00% <100.00%> (+100.00%)	⬆️
...workflow/src/manifests_workflow/input_manifests.py	95.65% <100.00%> (ø)
bundle-workflow/src/system/properties_file.py	97.36% <100.00%> (ø)
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c678193...0972e82. Read the comment docs.

[peternied]

What about defining a schema document and validating? Implemented something close on my fork Schematize and validate bundle manitests

[peternied]

Since python doesn't enforce typing I guess I am hesitant to rely on the current effort because while it works right now, a new untyped field could be added and it might be missed.

[dblock]

I like the schema idea better. Do you want to finish that or do you want me to take it?

[peternied]

@dblock Could you pick it up the schema change and make a new PR?

[dblock]

Closing in favor of #509.