utils: Don't change the name of the image, breaks the get_ami.py layout #94
Conversation
| @@ -238,7 +238,7 @@ def copy_images_to_regions(self, image_id=None, base_region=None, regions=None): | |||
| for region in regions: | |||
| log.info('Copy %s to %s started' % (image_id, region)) | |||
| self.set_region(region) | |||
| self.image_name = get_image_name_from_ami_name(image.name, region) | |||
There was a problem hiding this comment.
if we're going to remove this function call then we can remove the import of this function at the top of the file
fedimg/utils.py
Outdated
| @@ -154,8 +154,7 @@ def get_image_name_from_image(image_url, virt_type='', region='', respin='0', | |||
| file_name = get_file_name_image(image_url) | |||
| build_name = file_name.replace('.raw.xz', '') | |||
|
|
|||
| return '-'.join( | |||
| [x for x in [build_name, virt_type, region, volume_type, respin] if x]) | |||
| return build_name | |||
There was a problem hiding this comment.
If we are doing this then we need to change it so that the function doesn't require all of those arguments (that are no longer used and update any calls to this function).
fedimg/utils.py
Outdated
| @@ -154,8 +154,7 @@ def get_image_name_from_image(image_url, virt_type='', region='', respin='0', | |||
| file_name = get_file_name_image(image_url) | |||
| build_name = file_name.replace('.raw.xz', '') | |||
|
|
|||
| return '-'.join( | |||
| [x for x in [build_name, virt_type, region, volume_type, respin] if x]) | |||
| return build_name | |||
|
|
|||
|
|
|||
| def get_image_name_from_ami_name(image_name, region): | |||
There was a problem hiding this comment.
Does this function need to be updated?
There was a problem hiding this comment.
or rather,, looks like it needs to be deleted
sayanchowdhury
force-pushed
the
hotfix/change-image-name-format
branch
2 times, most recently
from
651bb22 to
bc34171
Compare
|
LGTM |
| @@ -164,3 +164,10 @@ def get_image_name_from_ami_name(image_name, region): | |||
|
|
|||
| return '-'.join( | |||
| [x for x in [name_vt, region, volume_type, respin] if x]) | |||
|
|
|||
|
|
|||
| def get_image_name_from_ami_name_for_fedmsg(image_name): | |||
There was a problem hiding this comment.
I don't really understand why we need a new function here? Why do we need to name the ami something different than Fedora-Cloud-Base-26-20180329.0.x86_64 ?
There was a problem hiding this comment.
Let's just name the ami Fedora-Cloud-Base-26-20180329.0.x86_64 too
There was a problem hiding this comment.
I kept the name similar to how we use to have earlier in AWS. We can change the name to Fedora-Cloud-Base-26-20180329.0.x86_64 but might need to add a field incase a AMI with the same name is already there.
Also, I think it's better to have the volume name included in the image name ie standard & gp2.
There was a problem hiding this comment.
yeah - I honestly don't really think people use the name that much. I think there is value in being able to search the same name across all regions and find all AMIs that were made from the same compose. I guess there are probably better ways to do that too
|
I'm cutting out a release. I'm merging this PR. Let's create an issue, if required I'll make the changes to the AMI name in the next release. |
Signed-off-by: Sayan Chowdhury <[email protected]>
sayanchowdhury
force-pushed
the
hotfix/change-image-name-format
branch
from
bc34171 to
3946ed1
Compare
Signed-off-by: Sayan Chowdhury [email protected]