Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

Add Google image formatting to CLI #122

Merged
merged 1 commit into from
Nov 17, 2022
Merged

Add Google image formatting to CLI #122

merged 1 commit into from
Nov 17, 2022

Conversation

F-X64
Copy link
Member

@F-X64 F-X64 commented Nov 17, 2022

Part of #106

@F-X64 F-X64 requested review from major and miyunari November 17, 2022 13:33
Copy link
Member

@major major left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! ✅

Copy link
Member

@miyunari miyunari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this task! :) Just added one small question

@@ -116,7 +116,7 @@ def format_image(image: dict[str, str]) -> dict[str, str]:
"""

arch = image["architecture"]
image_id = image["id"]
image_id = image["name"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was id not correct? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah while the GCP image properties do contain an "ID" tag it's not actually used to create a VM instance. The "name" in a Google image is what we consider the image id in AWS and Azure.. This is once again one of those weird differences between cloud providers.. tbh. I've only noticed when I was running the live CLI tests. Turns out, the "normalize_google_images" method doesn't even parse the id property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw. fixed this in the Schema definition for sake of consistency!

@major major merged commit 1d70df5 into main Nov 17, 2022
@major major deleted the add-gcp-formatting-to-cli branch November 17, 2022 19:41
@F-X64 F-X64 added this to the Convert and serve image data milestone Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants