code review #2

dustymabe · 2024-04-18T19:07:05Z

No description provided.

src/osbuild-manifests/aws_tag.py

dustymabe · 2024-04-18T19:18:51Z

src/osbuild-manifests/aws_tag.py

+        #     "snapshot": "snap-0c1ca4850fcd5e573"
+        # }]}
+        amis = data['amis']
+        for ami in amis:


before we update the tag it would be nice to check to see if the tag exists on the AMI first.

this will combine nicely with a dry-run feature.. in the case the user passed in --dry-run then if the tags didn't exist on the AMI we could print a statement saying would add tags "FedoraGroup=coreos" to ami-XXX in region foobar.

src/osbuild-manifests/aws_tag.py

dustymabe · 2024-04-19T20:31:34Z

src/osbuild-manifests/aws_tag.py

+    if tagExists:
+        print(f"{resourceId} already tagged with FedoraUser=coreos tag")
+    else:
+        addTag(resourceId, region)
+        print(f"FedoraUser=coreos tag successfully added to {resourceId}")    


I still think it would be nice to add a --dry-run option to this script and do something like:

Suggested change

if tagExists:

print(f"{resourceId} already tagged with FedoraUser=coreos tag")

else:

addTag(resourceId, region)

print(f"FedoraUser=coreos tag successfully added to {resourceId}")

if tagExists:

print(f"{resourceId} already tagged with FedoraUser=coreos tag")

return

if dryrun:

print(f"--dry-run specified. Would add FedoraUser=coreos tag to {resourceId}")

else:

addTag(resourceId, region)

print(f"FedoraUser=coreos tag successfully added to {resourceId}")

dustymabe · 2024-04-19T20:32:39Z

src/osbuild-manifests/aws_tag.py

+        print(f"FedoraUser=coreos tag successfully added to {resourceId}")    
+
+def checkTag(resourceId):
+    checkTagCmd = f'aws ec2 describe-tags --filters Name=resource-id,Values={resourceId} Name=value,Values=coreos'


what does the Name=value,Values=coreos do here?

Also do we need to add --region to this command too?

We don't need --region when we are giving --resource-id as an arg.

And for Name=value,Values=coreos, that's the way to filter out data where Name is basically "key" and Values is well "value" from tags json data that's spit out from aws ec2 describe tags
An example of what I get from this ami is this:

{ "Tags": [ { "Key": "FedoraUser", "ResourceId": "ami-xyx12345", "ResourceType": "image", "Value": "coreos" } ] }

src/osbuild-manifests/aws_tag.py

dustymabe · 2024-04-23T18:32:03Z

src/osbuild-manifests/aws_tag.py

+                amis = data['amis']
+            else:
+                print(f"{build_id} does not have any AMIs for {arch} in meta.json")
+                return


we probably shouldn't return here but rather continue the loop because we want to continue to process later builds and other architectures

dustymabe · 2024-04-23T18:40:22Z

src/osbuild-manifests/aws_tag.py

+        print(f"FedoraGroup=coreos tag successfully added to {resourceId}")    
+ 
+def checkTag(resourceId, region):
+    checkTagCmd = f'aws ec2 describe-tags --filters Name=resource-id,Values={resourceId} Name=value,Values=coreos Name=key,Values=FedoraGroup --region {region} --output=json'


Let's try with: Name=tag:FedoraGroup,Values=coreos, which is a bit easier to follow.

dustymabe · 2024-04-25T15:32:01Z

src/osbuild-manifests/aws_tag.py

+                amis = data['amis']
+            else:
+                print(f"{build_id} does not have any AMIs for {arch} in meta.json")
+                break


I think we need to continue and not break here? if we break then we won't check all architectures for a build ID

Suggested change

break

continue

dustymabe · 2024-04-25T15:32:35Z

src/osbuild-manifests/aws_tag.py

+    return
+
+def checkAndAddTag(resourceId, region, dry_run):
+    checkTagCmd = f'aws ec2 describe-tags --filters Name=resource-id,Values={resourceId} --region {region} --output=json'


optional: for clarity I would rename this:

Suggested change

checkTagCmd = f'aws ec2 describe-tags --filters Name=resource-id,Values={resourceId} --region {region} --output=json'

describeTagsCmd = f'aws ec2 describe-tags --filters Name=resource-id,Values={resourceId} --region {region} --output=json'

dustymabe · 2024-04-25T15:42:29Z

src/osbuild-manifests/aws_tag.py

+        else:
+            if dry_run:
+                print(f"Would add tag 'FedoraGroup=coreos' to {resourceId} in region {region}")
+                return
+            else:
+                addTag(resourceId, region)
+                print(f"FedoraGroup=coreos tag successfully added to {resourceId}")
+                return


We should be able to move this logic into addTag() then this becomes easier to read:

else: addTag(resourceId, region, dry_run)

dustymabe · 2024-04-25T15:42:46Z

src/osbuild-manifests/aws_tag.py

+    except subprocess.CalledProcessError as e:
+        return(e.output)


I think we should drop catching these exceptions

dustymabe · 2024-04-25T15:43:48Z

let's drop all try/catch blocks. We shouldn't be ignoring the exceptions I don't think. Is there any place where it makes sense for us to continue and ignore it?

dustymabe · 2024-04-25T17:34:03Z

src/osbuild-manifests/aws_tag.py

+
+
+def getBuildsForStream(stream):
+    buildFetch = 'cosa buildfetch --stream='+ stream + ' --arch=all'


maybe for consistency with other vars

Suggested change

buildFetch = 'cosa buildfetch --stream='+ stream + ' --arch=all'

buildFetchCmd = 'cosa buildfetch --stream='+ stream + ' --arch=all'

dustymabe · 2024-04-25T18:10:17Z

src/osbuild-manifests/aws_tag.py

+        if dry_run:
+            print(f"Would add tag 'FedoraGroup=coreos' to {resourceId} in region {region}")
+            return
+        else:                                                                                                                      
+            addTag(resourceId, region, dry_run)                                                                                    


Suggested change

if dry_run:

print(f"Would add tag 'FedoraGroup=coreos' to {resourceId} in region {region}")

return

else:

addTag(resourceId, region, dry_run)

addTag(resourceId, region, dry_run)

code review

6644cdd

dustymabe commented Apr 18, 2024

View reviewed changes

src/osbuild-manifests/aws_tag.py Outdated Show resolved Hide resolved

dustymabe commented Apr 18, 2024

View reviewed changes

src/osbuild-manifests/aws_tag.py Outdated Show resolved Hide resolved

dustymabe commented Apr 18, 2024

View reviewed changes

src/osbuild-manifests/aws_tag.py Outdated Show resolved Hide resolved

dustymabe commented Apr 18, 2024

View reviewed changes

src/osbuild-manifests/aws_tag.py Outdated Show resolved Hide resolved

fix

5eca0e4

dustymabe commented Apr 19, 2024

View reviewed changes

dustymabe force-pushed the master branch from 15d6820 to af976fa Compare April 23, 2024 18:25

dustymabe commented Apr 23, 2024

View reviewed changes

gursewak1997 force-pushed the dusty-gursewak branch from efcfd1b to 2144e8f Compare April 25, 2024 06:00

dustymabe commented Apr 25, 2024

View reviewed changes

gursewak1997 force-pushed the dusty-gursewak branch from 2144e8f to 26f4307 Compare April 25, 2024 17:13

dustymabe commented Apr 25, 2024

View reviewed changes

gursewak1997 force-pushed the dusty-gursewak branch from 26f4307 to c2ba0a9 Compare April 25, 2024 17:35

Updated changes

179bfd6

gursewak1997 force-pushed the dusty-gursewak branch from c2ba0a9 to 179bfd6 Compare April 25, 2024 17:36

dustymabe commented Apr 25, 2024

View reviewed changes

gursewak1997 force-pushed the dusty-gursewak branch from 179bfd6 to fd814ca Compare April 25, 2024 18:17

fix

4f37b1f

dustymabe force-pushed the dusty-gursewak branch from fd814ca to 4f37b1f Compare April 28, 2024 14:46

gursewak1997 approved these changes Apr 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

code review #2

code review #2

dustymabe commented Apr 18, 2024

dustymabe Apr 18, 2024

dustymabe Apr 19, 2024

dustymabe Apr 19, 2024

gursewak1997 Apr 22, 2024 •

edited

Loading

dustymabe Apr 23, 2024

dustymabe Apr 23, 2024

dustymabe Apr 25, 2024

dustymabe Apr 25, 2024

dustymabe Apr 25, 2024

dustymabe Apr 25, 2024

dustymabe commented Apr 25, 2024

dustymabe Apr 25, 2024

dustymabe Apr 25, 2024

	checkTagCmd = f'aws ec2 describe-tags --filters Name=resource-id,Values={resourceId} --region {region} --output=json'
	describeTagsCmd = f'aws ec2 describe-tags --filters Name=resource-id,Values={resourceId} --region {region} --output=json'



		def getBuildsForStream(stream):
		buildFetch = 'cosa buildfetch --stream='+ stream + ' --arch=all'

	buildFetch = 'cosa buildfetch --stream='+ stream + ' --arch=all'
	buildFetchCmd = 'cosa buildfetch --stream='+ stream + ' --arch=all'

code review #2

Are you sure you want to change the base?

code review #2

Conversation

dustymabe commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gursewak1997 Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustymabe commented Apr 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gursewak1997 Apr 22, 2024 •

edited

Loading