SCIPROD-432 SCIPROD-499 SCIPROD-540 SCIPROD-560: CAWFR dataset utilities #786
Conversation
…SCIPROD-432_CAWFR_dataset_utilities
…SCIPROD-432_CAWFR_dataset_utilities
…features in write function to be added
…SCIPROD-432_CAWFR_dataset_utilities
…r handling, minor modifications in code structure
…SCIPROD-432_CAWFR_dataset_utilities
…name creation logic
…SCIPROD-432_CAWFR_dataset_utilities
…d and --output parameters
…ith dataset_name_prefix
…und exception logic
…n logic for double and float. Added logic to handle cohortbrowser input
…input validations
…SCIPROD-432_CAWFR_dataset_utilities
…outing for --fields
| if not print_to_stdout: | ||
| fields_output.close() | ||
|
|
||
| class DXDataset(DXRecord): |
There was a problem hiding this comment.
In this PR I think it's fine to leave in this file as the classes are not used elsewhere within dxpy like the other DNAnexus object bindings.
| def end_to_end_ddd(self, out_directory, rec_name): | ||
| truth_files_directory = tempfile.mkdtemp() | ||
| os.chdir(truth_files_directory) | ||
| cmd = ["dx", "download", "project-G9j1pX00vGPzF2XQ7843k2Jq:file-GBGp61j0vGPpz58XK9f7Vkj6", |
There was a problem hiding this comment.
Not planning to make the staging project public. @commandlinegirl given that this functionality is isolated from the rest of dx-toolkit I'm okay with the current approach, although perhaps we should set up the same data in prod and then change these tests to refer to project_name:filename instead of object IDs.
| print("Insufficient permissions") | ||
| sys.exit(1) |
There was a problem hiding this comment.
err_exit() can be provided expected exceptions and then will only print the stacktrace if dxpy._DEBUG > 1
e.g.
dx-toolkit/src/python/dxpy/scripts/dx.py
Lines 1491 to 1492 in d321b31
Replaced sys.exit(1) with err_exit Replaced 'raise err_exit' with 'err_exit'
…SCIPROD-432_CAWFR_dataset_utilities
|
@Niranjanpandeshwar, Looks good to me, very nice! |
| if resp_raw['error']['details'] and 'PermissionDenied: Access to the specified database is not allowed' in resp_raw['error']['details']: | ||
| print("Insufficient permissions due to the project policy") | ||
| if resp_raw['error']['type'] == 'InvalidInput': | ||
| print("Insufficient permissions due to the project policy.") |
There was a problem hiding this comment.
"project policy" is not too informative. Should it say "project access policy"? Or which policy is it about? The wording should be such that the user ideally can quickly understand by a quick search in our documentation.
There was a problem hiding this comment.
There was a problem hiding this comment.
The error messages came originally from PM with security approval on not specifying project policy. We are currently checking 3 different project policies and not specifying which one caused the error in any other cases.
I'm just trying to explain the origin of those error messages, not that they are correct or we should do it like that. Maybe @keegankelsey can chime in?
There was a problem hiding this comment.
If it's general on purpose then it can stay as is. Thanks for providing more context @durae!
There was a problem hiding this comment.
I don't have information as to the rationale for keeping this general. However, if we think of a malicious actor (probably rare use case) then it would be good to minimize any targeted information for follow up on the part of the actor. So, I think keeping it general is fine.
Co-authored-by: Aleksandra Zalcman <[email protected]>
Co-authored-by: Aleksandra Zalcman <[email protected]>
Co-authored-by: Aleksandra Zalcman <[email protected]>
Co-authored-by: Aleksandra Zalcman <[email protected]>
Co-authored-by: Aleksandra Zalcman <[email protected]>
There was a problem hiding this comment.
LGTM
Thanks very much @Niranjanpandeshwar for all the great work on this!
Adding new command
dx extract_datasetto dx-toolkit.