-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat(MIG-166): throw manifest not found error #500
Conversation
@@ -34,6 +34,10 @@ class StudyDataTranslations { | |||
} | |||
} | |||
|
|||
if (!manifestData) { | |||
throw Fault.create('mdctl.kManifestNotFound.error', { reason: 'There is no manifest set as parameter neither found in directory' }) |
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.
The error message seems ambiguous to me, what about this?
No manifest found in the parameters and in the import directory
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.
Sounds the same to me
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.
I guess the grammatically correct version should be "There is no manifest set as a parameter, nor is one found in the directory."
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.
@imaimai86 @joejean just updated the error message as u guys suggested.
Hi @abulseed we've released 1.0.71, can you branch out of it? |
@alahawash it seems that the changes related to |
Ohh that's awful, we've got to reconcile those branches. Can this MR wait? |
yes no worries this can wait. Sounds good please let me know. |
Hi @abulseed , please rebase from |
9817a93
to
5f08480
Compare
8dc76d1
to
5a0aa4e
Compare
@alahawash just rebased from are we sure we have merged |
You are right @abulseed , when I merged before my local wasn't up-to-date. I did merge again and you should not have any issues now if you rebase from main. Sorry for the that |
84d398c
to
5a18d9b
Compare
@alahawash done! please review.. |
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
Follow up PR to resolve an issue with the error code that is thrown if there is no manifest file provided to the import command.