-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issues with archive_catalog
marked keywords not being required
#505
base: main
Are you sure you want to change the base?
Fix issues with archive_catalog
marked keywords not being required
#505
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #505 +/- ##
==========================================
+ Coverage 95.81% 96.05% +0.23%
==========================================
Files 4 4
Lines 215 228 +13
==========================================
+ Hits 206 219 +13
Misses 9 9 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. It's good to see that sw_version and context_used were the most important problems here.
I don't think we want to touch the FPS & TVAC schemas. I don't actually know if those are affected by the "required" changes but if one of those fields were left out, we don't want to invalidate those files now; those schemas are fixed.
For the changes to the non-frozen schemas, that looks possibly right to me, except that inverse_linearity will never be set in practice and probably should not have been assigned an archive_catalog. I don't know well enough if we always set all of those keywords. Let's not merge this for the upcoming release this week.
But it's great to see that the two sw_version and context_used are the only things that got missed in the intermediate release, thanks!
…certain metadata attributes that it is forced to be required
1bebd16
to
5dac4b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I will approve once the FPS & TVAC changes are removed.
Closes #502
Closes #503
In order to walk the schemas to find
archive_catalog
information, a validAsdfFile
object must exist satisfying those schemas. Thus in order to discover this metadata, all those properties must be present meaning that they must be required by the schemas.This PR adds a test to check that
archive_catalog
(andsdf
) force the keyword to be required. Moreover, this PR fixes all the failures detected by this process.Tasks
rad
tests.docs/
page.no-changelog-entry-needed
.)changes/
:echo "changed something" > changes/<PR#>.<changetype>.rst
(see below for change types).romancal
regression test (https://github.com/spacetelescope/RegressionTests/actions/workflows/romancal.yml) with this branch installed ("git+https://github.com/<fork>/rad@<branch>"
).roman_datamodels
utilities and tests.News fragment change types:
changes/<PR#>.feature.rst
: new featurechanges/<PR#>.bugfix.rst
: fixes an issuechanges/<PR#>.doc.rst
: documentation changechanges/<PR#>.removal.rst
: deprecation or removal of public APIchanges/<PR#>.misc.rst
: infrastructure or miscellaneous change