-
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
Delete s3 Objects and items related to an execution #173
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.
@caviri just some small change request: the most important one: make providing the project-path optional.
Hello @sabinem, I addressed your suggestions. Would you make to take another look? |
@caviri I don't think you made the |
dab4682
to
a3da51c
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.
@caviri I approve now.
But I would still suggest that you improve a bit on the output of the command:
/app # odtp execution delete --execution-id 6683ed98b66b18232839f9d0
odtp/6683ed98b66b18232839f9d1/
odtp/6683ed98b66b18232839f9d1/odtp-output.zip
Could you please improve the output: I would rather expect a message here that the execution about what exactly has been deleted.
I left some comments in the code on this issue: the log statements write now to file for the execution commands, therefore you need to use print statements in the cli command for both exception and success cases.
Please remove the print
from storage.py
.
TODOs:
- remove
print
instorage.py
- add
print
incli/execution.py
and uselog.exception
instead oflog.error
since it gives you a traceback.
odtp/cli/execution.py
Outdated
log.error(f"ERROR: Delete execution failed: {e}") | ||
raise typer.Abort() | ||
else: | ||
log.info("SUCCESS: execution has been deleted") |
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 did not get this message in my tests. I got this instead:
odtp/6683af7d5923cfd2e73e613a/
odtp/6683af7d5923cfd2e73e613a/odtp-output.zip
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 tested with the last rebase from develop and it works fine:
03/07/2024 10:42:20 AM - [storage:INFO] 140 storage.py deletePaths - Path 'odtp/66852a00d5b566c9b0c1b4cd' deleted from S3 bucket
03/07/2024 10:42:21 AM - [environment:INFO] 72 environment.py delete_folder - Folder deleted: /Users/carlosvivarrios/pro/ODTP/odtp-instance/dts/dt-example/dt-example/execution
03/07/2024 10:42:21 AM - [execution:INFO] 165 execution.py delete - SUCCESS: execution has been deleted
SUCCESS: execution has been deleted
odtp/cli/execution.py
Outdated
log.error(f"ERROR: Delete execution failed: {e}") | ||
raise typer.Abort() | ||
else: | ||
log.info("SUCCESS: execution has been deleted") |
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.
Add print statements in the cli command, since the log statement will only go to file
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 for the rewiew @sabinem, I see the logic now.
I'll follow this, but this should be change as In the future I would like to use print statements in the CLI to pipe CLI commands (Not implemented yet).
Ideally only log.X() should go to the log file, otherwise it's a bit confusing.
dd54145
to
0421d18
Compare
These methods allows to delete all items related to an execution including s3 objects. It updates the digital twin document removing the reference to the execution document.