services.ec2: Delete the source for every image urls #112
Conversation
|
@dustymabe Can you review this? |
| @@ -24,6 +24,8 @@ | |||
|
|
|||
| import re | |||
|
|
|||
| from time import sleep | |||
|
|
|||
There was a problem hiding this comment.
i don't understand why this is removed. things in this file use it
There was a problem hiding this comment.
This was not removed, this was added.
There was a problem hiding this comment.
oops. i guess my diff view confused me on that. Can you put this in a separate commit?
fedimg/services/ec2/ec2initiate.py
Outdated
| @@ -163,5 +163,8 @@ def main(image_urls, access_id, secret_key, regions, volume_types=None, | |||
| #TODO: Implement the clean up of the images if failed. | |||
| # uploader.clean_up(image_id=image.id, delete_snapshot=True) | |||
|
|
|||
| shutil.rmtree(os.path.dirname(source)) | |||
| if source: | |||
There was a problem hiding this comment.
in what cases does source evaluate to non true? I don't see what problem this is solving.
There was a problem hiding this comment.
There could be various reasons when source evaluates to non true, euca2ools does not setup, or wget fails. Though, fedimg raises ValueError if source is None but the handlers are not in place.
fedimg needs to be better at raising and catching exceptions. I started working on them this week and I have also opened up #113 for the same.
There was a problem hiding this comment.
it looks like this should be in a finally block then. If the valueError gets raised above then this code wouldn't get hit right?
sayanchowdhury
force-pushed
the
hotfix/105-remove-images-after-upload
branch
from
925f871
to
bebd8bd
Compare
fedimg/services/ec2/ec2initiate.py
Outdated
| except Exception as e: | ||
| _log.debug(e.message) | ||
| return [] |
There was a problem hiding this comment.
so here we are returning [] where we weren't sending anything back before. wehere is this main function getting called from?
There was a problem hiding this comment.
This is getting called from here: https://github.com/fedora-infra/fedimg/blob/develop/fedimg/uploader.py#L57
fedimg/services/ec2/ec2initiate.py
Outdated
| @@ -158,10 +158,16 @@ def main(image_urls, access_id, secret_key, regions, volume_types=None, | |||
| published_images.extend(publisher.publish_images( | |||
| region_image_mapping=[(region, image.id)] | |||
| )) | |||
| except ValueError: | |||
| _log.debug('Source not found.') | |||
| return [] | |||
There was a problem hiding this comment.
why not just do this on line 94 above where we raise the ValueError ?
There was a problem hiding this comment.
Actually, my plan for raising exception is to remove anything that happened before. I'll just make changes according to your comments for now.
There was a problem hiding this comment.
... Later when working on better exception handling, I'll look into it.
sayanchowdhury
force-pushed
the
hotfix/105-remove-images-after-upload
branch
from
bebd8bd
to
b6379e4
Compare
| @@ -91,7 +91,8 @@ def main(image_urls, access_id, secret_key, regions, volume_types=None, | |||
| try: | |||
| source = get_source_from_image(image_url) | |||
| if not source: | |||
| raise ValueError | |||
| _log.debug('Source not found.') | |||
| return [] | |||
There was a problem hiding this comment.
this looks good for now but in the future I hope to get more output in the logs other than Source not found.. I opened #118 for this
|
LGTM |
Signed-off-by: Sayan Chowdhury <[email protected]>
sayanchowdhury
force-pushed
the
hotfix/105-remove-images-after-upload
branch
from
b6379e4
to
bf6450c
Compare
Signed-off-by: Sayan Chowdhury [email protected]