code review by dustymabe · Pull Request #2 · dustymabe/coreos-assembler

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

No description provided.

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

+        #     "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.

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

+    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

+        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" } ] }

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

+                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

+        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

+                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

+    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

+        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

+    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

+
+
+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

+        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

Comment thread src/osbuild-manifests/aws_tag.py Outdated

dustymabe commented Apr 18, 2024

View reviewed changes

Comment thread src/osbuild-manifests/aws_tag.py Outdated

dustymabe commented Apr 18, 2024

View reviewed changes

Comment thread src/osbuild-manifests/aws_tag.py Outdated

dustymabe commented Apr 18, 2024

View reviewed changes

Comment thread src/osbuild-manifests/aws_tag.py Outdated

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

	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'

Conversation

dustymabe commented Apr 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gursewak1997 Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dustymabe commented Apr 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gursewak1997 Apr 22, 2024 •

edited

Loading