Add tag while uploading rhcos images #35
Conversation
|
Have you considered to use Lines 539 to 540 in b416dc0 or similar for azure? |
| """ | ||
| Create copy of an AMI. | ||
|
|
||
| Args: | ||
| image_id (str): AMI Id of the image to copy | ||
| name (str): Name of new image | ||
| region(str): Region for new Image. | ||
| tags(list, optional): Tags to be associated with new ami. |
There was a problem hiding this comment.
It's documented as optional, but it's not actually optional in the python sense - copy_ami now requires a value for tags, and 100% of existing callers won't be providing that, so they'll just crash. This is a severe backwards-incompatible change.
It needs to be made genuinely optional.
There was a problem hiding this comment.
This comment still seems to be valid: if someone calls this method without providing tags, it'll crash, so it's still not backwards-compatible.
What I'm saying is that the signature should have:
tags=None
tests/test_aws.py
Outdated
| @@ -365,10 +365,20 @@ def test_copy_ami(self): | |||
| self.mock_copy_ami.return_value = { | |||
| 'ImageId': 'ami-08', | |||
| 'ResponseMetadata': {'RequestId': 'cf32', 'server': 'AmazonEC2'}} | |||
| copy_ami_result = self.svc.copy_ami("ami-01", "rhcos", "us-east-1") | |||
| self.assertEqual(copy_ami_result['ImageId'], "ami-08") | |||
| copy_ami_result = self.svc.copy_ami( | |||
There was a problem hiding this comment.
Minor: rather than changing the only test which calls copy_ami so that it passes the new argument, it would be nice to instead add a new call so that there is coverage of both passing and not passing tags.
This relates to my earlier comment about the argument wrongly not being optional. That'll cause existing callers of copy_ami to crash. The test not passing tags accurately reproduces what any existing caller of copy_ami was doing, so the test would have been able to catch this crash.
Changing the test so it always passes tags means there is no longer any test which reproduces what the current users of copy_ami are doing, hiding a potential source of crash that real users would encounter.
There was a problem hiding this comment.
Changes it to support cases with tags as None. PTAL
ashwini3326
force-pushed
the
add_tags_with_rhcos_upload
branch
from
030f666 to
3bba175
Compare
|
As this issue is fixed by another merge reqest to pubtools-marketplacesvm , it is not required now. |
This merge request enables adding tags to images being uploaded to aws and azure. @JAVGan @jajreidy @rohanpm PTAL.