-
Notifications
You must be signed in to change notification settings - Fork 1
images info for release notes #217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
==========================================
+ Coverage 87.62% 92.37% +4.74%
==========================================
Files 33 40 +7
Lines 1543 1822 +279
==========================================
+ Hits 1352 1683 +331
+ Misses 191 139 -52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| """ | ||
| Generate the table format with collapsible region details | ||
| """ | ||
| output = "| Variant | Platform | Architecture | Flavor | Regions & Image IDs | Download Links |\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use a module for this? Could something like PrettyTable or pytable be a better solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be probably (maybe) easier to read, but I'm not sure if need to bring another dependency because of that.
| @@ -0,0 +1,12 @@ | |||
| from . import DeploymentPlatform | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not be a dataclass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could very well be.
| "usi": "USI (Unified System Image)", | ||
| "tpm2_trustedboot": "TPM2 Trusted Boot", | ||
| } | ||
| PLATFORMS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an interesting take. Maybe we should add some documentation about why we do this? Couldn't we create this not with enums instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dict is used here simply for grouping platform objects.
| if variant not in grouped_data: | ||
| continue | ||
|
|
||
| for platform in sorted(grouped_data[variant].keys()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could write more effective code here by using the itertools module instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, but suggest to do it as a separate improvement PR.
| continue | ||
|
|
||
| output += ( | ||
| f"<details>\n<summary>Variant - {IMAGE_IDS_VARIANT_NAMES[variant]}</summary>\n\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Python 3.14, they stabilized the template string. I feel like I might benefit from using it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it in mind, but 3.14 is too fresh to demand it from the library users.
| return "openstackbaremetal" | ||
|
|
||
| def full_name(self): | ||
| return "OpenStack Baremetal" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we miss the published_images_by_regions here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some platforms superclass' version is used.
|
|
||
|
|
||
| def test_script_parse_args_wrong_command(monkeypatch, capfd): | ||
| monkeypatch.setattr(sys, "argv", ["gh", "rejoice"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we call this tool gh? That is confusing, and we have to change it; otherwise, it will collide with the gh cli tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this command name is used just for testing. The actual script name for releases creation has to be added to pyproject.toml's tool.poetry.scripts section.
What this PR does / why we need it:
Splitting #179 into smaller parts
Which issue(s) this PR fixes:
gardenlinux/gardenlinux#3054