-
Notifications
You must be signed in to change notification settings - Fork 1
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 issue 152 #155
Fix issue 152 #155
Conversation
…-catalog-ingestor/tree/fix-issue-153 and follow the logic for the parent namespace swap, add test
managed 100% coverage I hope it makes sense |
added a return (+ test) that should fix #156 |
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 checked out this branch and run the pytest and code-coverage tests, and everything sucess and 100%. However, when I try to ingest some mmd using catalog-rebuilder which is have
Looks like there are missing return parameter on line |
I added the check at line 125 for pure redundancy, because we said the fact that a parent does not have a correct identifier should not be possible once we fix py-mmd-tools... but apparently we need it 😅 I will edit the return, you are right that it misses the 3rd argument
(and similar for the test of line 125, written to expect 2 params in return) I will fix this too |
@magnarem can you try again? |
Tested locally with a few test-mmd files. Works ok now. |
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.
Reviewed and approved
NOTE!!!!!! If we merge this issue now, before solving #242 (py-mmd-tools), all incoming MMD files that have Reason: Input MMD XML file has no valid uri:UUID metadata_identifier So maybe we sould discuss tomorrow morning meeting if we should solve #242 before we merge this. Or if we are ok with new incoming data not stored in the archives, then we can merge when we want. |
Sounds best to wait :)
|
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.
Approving this MR. But we should probably still wait until #242-py-mmd-tools have been merged.
Summary: fixes #152
Related issue:
Suggested reviewer(s):
Reviewer checklist:
The checklist is based on the S-ENDA conventions and definition of done (see General Conventions). The above points are not necessarily relevant to all contributions. In that case, please add a short explanation to help the reviewer.