-
Notifications
You must be signed in to change notification settings - Fork 4
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
EVA-3590 update initial check for validation required or not #40
EVA-3590 update initial check for validation required or not #40
Conversation
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've put some suggestions but overall this looks fine.
eva_sub_cli/main.py
Outdated
return False | ||
except requests.HTTPError as ex: | ||
logger.error(f'Error occurred while getting status of the submission with ID {submission_id}: {ex.response.status_code} {ex.response.text}') | ||
raise |
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 wonder if we should not raise anything here and return true/false depending on the status code and what's safe to do (i.e. 404 should behave like no ID found I guess). Otherwise we need to clearly convey what the user should do when this crashes - try again later? Email helpdesk?
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 see two options:
- Have a sensible default when the API fails to respond ie submission does not exist
- Raising an error that should be caught and exit gracefully with a clear error message
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.
raising and existing gracefully with msg
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.
Approach looks good to me.
Couple of cosmetic points
eva_sub_cli/main.py
Outdated
if SUBMIT in tasks: | ||
if not sub_config.get(READY_FOR_SUBMISSION_TO_EVA, False): | ||
return True | ||
else: |
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.
else:
is unnecessary since you return in the if
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.
updated
eva_sub_cli/main.py
Outdated
except requests.HTTPError as ex: | ||
logger.error(f'Error occurred while getting status of the submission with ID {submission_id}: {ex.response.status_code} {ex.response.text}') | ||
raise | ||
else: |
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.
Again else:
is not necessary
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.
updated
eva_sub_cli/main.py
Outdated
return False | ||
except requests.HTTPError as ex: | ||
logger.error(f'Error occurred while getting status of the submission with ID {submission_id}: {ex.response.status_code} {ex.response.text}') | ||
raise |
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 see two options:
- Have a sensible default when the API fails to respond ie submission does not exist
- Raising an error that should be caught and exit gracefully with a clear error message
bin/eva-sub-cli.py
Outdated
@@ -83,3 +86,7 @@ def get_version(): | |||
main.orchestrate_process(**args.__dict__) | |||
except FileNotFoundError as fne: | |||
print(fne) | |||
except SubmissionNotFoundException as snfe: | |||
print(f'{snfe}. Retry with correct submission id. If the problem persists, please contact EVA Helpdesk') |
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'm not sure the user will know the submission ID since it's just stored in the config, there's not a command line option to set it or anything. Since this is just when it's not found in the WS maybe just contact helpdesk in this case.
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.
updated
eva_sub_cli/submission_ws.py
Outdated
|
||
@property | ||
def _submission_ws_url(self): | ||
"""Retrieve the base URL for the submission web services. In order of preference from the user of this class, |
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.
"""Retrieve the base URL for the submission web services. In order of preference from the user of this class, | |
"""Retrieve the base URL for the submission web services. In order of preference from |
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.
updated
No description provided.