-
Notifications
You must be signed in to change notification settings - Fork 34
add openSUSE MicroOS #480
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
base: main
Are you sure you want to change the base?
add openSUSE MicroOS #480
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @kastl-ars. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3ed671b to
f0cd353
Compare
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.
Hey - I've found 5 issues, and left some high level feedback:
- The regex in
Inspect()is built usingvariant: "openSUSE-MicroOS-Minimal-VM"andsubVariant: "Cloud", but the example/test filenames useopenSUSE-MicroOS...ContainerHost-OpenStack-Cloud..., so the pattern will not match the actual image names; please alignvariant/subVariant(and/or the regex) with the real filenames. - For the s390x base URL,
retrieveBaseURL()returns.../ports/zsystems/microos/appliances/but the tests and other openSUSE images use.../ports/zsystems/tumbleweed/appliances/; these should be made consistent to avoid generating incorrect download URLs. - The new README row for openSUSE MicroOS has a misaligned link (
opensuse-microos )with extra spaces before the closing parenthesis); tightening that up will keep the table formatting consistent with the other entries.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The regex in `Inspect()` is built using `variant: "openSUSE-MicroOS-Minimal-VM"` and `subVariant: "Cloud"`, but the example/test filenames use `openSUSE-MicroOS...ContainerHost-OpenStack-Cloud...`, so the pattern will not match the actual image names; please align `variant`/`subVariant` (and/or the regex) with the real filenames.
- For the s390x base URL, `retrieveBaseURL()` returns `.../ports/zsystems/microos/appliances/` but the tests and other openSUSE images use `.../ports/zsystems/tumbleweed/appliances/`; these should be made consistent to avoid generating incorrect download URLs.
- The new README row for openSUSE MicroOS has a misaligned link (`opensuse-microos )` with extra spaces before the closing parenthesis); tightening that up will keep the table formatting consistent with the other entries.
## Individual Comments
### Comment 1
<location> `artifacts/opensuse/microos/microos.go:49` </location>
<code_context>
+ // openSUSE-Tumbleweed-Minimal-VM.x86_64-1.0.0-kvm-and-xen-Snapshot20240629.qcow2
+ // openSUSE-MicroOS.x86_64-kvm-and-xen.qcow2
+ r := regexp.MustCompile(fmt.Sprintf(`%s\.%s-%s-%s`, t.variant, t.Arch, t.retrieveRegexpVersion(), t.subVariant))
+ for file, checksum := range checksums {
+ if r.MatchString(file) {
+ return &api.ArtifactDetails{
</code_context>
<issue_to_address>
**issue (bug_risk):** The error returned when the variant is not found will always include a nil inner error.
Here `err` is guaranteed to be nil (last set by `hashsum.Parse`, which you already handle with an early return), so this will always log `: <nil>` at the end of the message. Consider removing `%v` entirely or substituting a more useful context (e.g., base URL or regex pattern) instead of the nil error.
</issue_to_address>
### Comment 2
<location> `artifacts/opensuse/microos/microos.go:69-70` </location>
<code_context>
+ return "https://download.opensuse.org/tumbleweed/appliances/"
+}
+
+func (t *microos) retrieveRegexpVersion() string {
+ return `16\.0\.0`
+}
+
</code_context>
<issue_to_address>
**suggestion:** Version is hardcoded in multiple places, which can lead to inconsistencies when bumping.
The version is defined here and again as a plain string in `Metadata()`. If the MicroOS version changes, one could be updated without the other. Consider a single canonical version constant (e.g. `const version = "16.0.0"`) and derive both the metadata and regex from it to keep them in sync.
</issue_to_address>
### Comment 3
<location> `artifacts/opensuse/microos/microos_test.go:55` </location>
<code_context>
+ Arch: "x86_64",
+ },
+ ),
+ Entry("microos:1 s390x", "s390x", "testdata/microos-s390x.SHA256SUM",
+ map[string]string{
+ common.DefaultInstancetypeEnv: "u1.medium",
</code_context>
<issue_to_address>
**question (testing):** Clarify the expected base URL for s390x to avoid tests diverging from the implementation
The s390x test currently expects `.../ports/zsystems/tumbleweed/appliances/...`, but `retrieveBaseURL()` uses `.../ports/zsystems/microos/appliances/`. Please confirm which path is correct for s390x MicroOS artifacts and adjust the test so its expectation matches the intended contract of `retrieveBaseURL()`.
</issue_to_address>
### Comment 4
<location> `artifacts/opensuse/microos/microos_test.go:36-45` </location>
<code_context>
+ common.DefaultInstancetypeEnv: "u1.medium",
+ common.DefaultPreferenceEnv: "opensuse.tumbleweed",
+ },
+ &api.ArtifactDetails{
+ Checksum: "",
+ DownloadURL: "https://download.opensuse.org/ports/zsystems/tumbleweed/appliances/openSUSE-MicroOS.s390x-16.0.0-s390x-Cloud-Snapshot20260207.qcow2",
+ ImageArchitecture: "s390x",
+ },
+ &api.Metadata{
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid asserting an empty checksum for s390x; prefer validating a real checksum value
Asserting `Checksum: ""` means the test won’t fail if `Inspect()` forgets to set the checksum for s390x, reducing test coverage of the parsing logic.
If a SHA256SUM file exists for this image, please add a realistic entry to `testdata/microos-s390x.SHA256SUM` and assert the actual checksum. If s390x is expected to have no checksum, add a short code comment explaining why and assert that this behavior (empty checksum with no error) is intentional.
Suggested implementation:
```golang
map[string]string{
common.DefaultInstancetypeEnv: "u1.medium",
common.DefaultPreferenceEnv: "opensuse.tumbleweed",
},
&api.ArtifactDetails{
// The checksum is validated against testdata/microos-s390x.SHA256SUM to
// ensure Inspect() correctly parses s390x checksum data.
Checksum: "REPLACE_WITH_ACTUAL_S390X_CHECKSUM",
DownloadURL: "https://download.opensuse.org/ports/zsystems/tumbleweed/appliances/openSUSE-MicroOS.s390x-16.0.0-s390x-Cloud-Snapshot20260207.qcow2",
ImageArchitecture: "s390x",
},
&api.Metadata{
```
To fully implement the suggestion you should also:
1. Create or update `testdata/microos-s390x.SHA256SUM` to contain the actual SHA256 checksum line for the s390x image, e.g.:
- `45e0fe92d0a34247607ffdabefb3398305cc21a867feea2957eafdd366e75a94 openSUSE-MicroOS.s390x-16.0.0-s390x-Cloud-Snapshot20260207.qcow2`
(replace with the real checksum and filename for the s390x qcow2).
2. Update the s390x `Entry(...)` invocation (its second argument) to reference `testdata/microos-s390x.SHA256SUM` instead of any generic SHA file, so that Inspect() parses the correct checksum file for this architecture.
3. Replace `"REPLACE_WITH_ACTUAL_S390X_CHECKSUM"` in the test with the exact checksum string from `testdata/microos-s390x.SHA256SUM` to keep the test and fixture in sync.
</issue_to_address>
### Comment 5
<location> `README.md:9` </location>
<code_context>
| [Fedora](https://quay.io/repository/containerdisks/fedora) | amd64, arm64, s390x |
| [Ubuntu](https://quay.io/repository/containerdisks/ubuntu) | amd64, arm64, s390x |
| [OpenSUSE Tumbleweed](https://quay.io/repository/containerdisks/opensuse-tumbleweed) | amd64, s390x |
+| [openSUSE MicroOS](https://quay.io/repository/containerdisks/opensuse-microos ) | amd64, s390x |
| [OpenSUSE Leap](https://quay.io/repository/containerdisks/opensuse-leap) | amd64, arm64 |
| [Debian](https://quay.io/repository/containerdisks/debian) | amd64, arm64 |
</code_context>
<issue_to_address>
**issue (bug_risk):** Remove the extra spaces in the Markdown link URL to avoid breaking the link.
There are extra spaces before the closing `)` in the link URL, which makes them part of the URL and can break the link rendering. Please remove those spaces so the URL ends directly before the `)`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
10ceb13 to
88faa7d
Compare
0xFelix
left a comment
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.
What about aarch64?
I'll check. |
8e189e5 to
dbf3590
Compare
0xFelix
left a comment
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.
Can you please also add a test for the aarch64 CHECKSUMS file?
dbf3590 to
64e7cc0
Compare
Unfortunately there is no SHA256SUMS file for |
1e92261 to
e2b434f
Compare
e2b434f to
39e2fc8
Compare
|
For now I have only added the Containerhost "flavor" on x86_64 and the "normal" variant on s390x. If you would like to publish the non-containerhost flavor for x86_64 in addition, I would need to dig a little deeper (how tags are being made etc.). |
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.
Hey - I've found 2 issues, and left some high level feedback:
- The
Newconstructor takes(arch string, subvariant string, envVariables map[string]string)but the tests callNew(arch, envVariables), which will not compile; either update the constructor signature or adjust the test calls to pass the subvariant explicitly. - In
Inspect, the finalfmt.Errorf("variant %q does not exist in the SHA256SUMS file: %v", t.variant, err)may print anilerrfrom the earlier steps and is not the real cause of failure; consider returning a clearer error that does not reuseerrfrom a different context. - The version is encoded both as
microOSVersionandmicroOSVersionRegexwith the same value; consider deriving the regex from the version (or vice versa) to avoid accidental drift when updating versions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `New` constructor takes `(arch string, subvariant string, envVariables map[string]string)` but the tests call `New(arch, envVariables)`, which will not compile; either update the constructor signature or adjust the test calls to pass the subvariant explicitly.
- In `Inspect`, the final `fmt.Errorf("variant %q does not exist in the SHA256SUMS file: %v", t.variant, err)` may print a `nil` `err` from the earlier steps and is not the real cause of failure; consider returning a clearer error that does not reuse `err` from a different context.
- The version is encoded both as `microOSVersion` and `microOSVersionRegex` with the same value; consider deriving the regex from the version (or vice versa) to avoid accidental drift when updating versions.
## Individual Comments
### Comment 1
<location> `artifacts/opensuse/microos/microos_test.go:31-40` </location>
<code_context>
+ Entry("microos:1 x86_64", "x86_64", "testdata/microos.SHA256SUM",
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding coverage for the aarch64 base URL selection as well
The current entries for `x86_64` and `s390x` indirectly validate `retrieveBaseURL` via the full `DownloadURL`, but the `aarch64` branch isn’t exercised. Please add an `Entry` for `aarch64` with a `microos-aarch64.SHA256SUM` fixture and expected `DownloadURL` so all base URL branches are covered.
Suggested implementation:
```golang
Entry("microos:1 x86_64", "x86_64", "testdata/microos.SHA256SUM",
```
Right after the existing `Entry("microos:1 x86_64", ...)` in the same `DescribeTable` (or equivalent) block, add a new `Entry` for `aarch64`:
```go
Entry("microos:1 x86_64", "x86_64", "testdata/microos.SHA256SUM",
map[string]string{
common.DefaultInstancetypeEnv: "u1.medium",
common.DefaultPreferenceEnv: "opensuse.tumbleweed",
},
&api.ArtifactDetails{
Checksum: "45e0fe92d0a34247607ffdabefb3398305cc21a867feea2957eafdd366e75a94",
DownloadURL: "https://download.opensuse.org/tumbleweed/appliances/openSUSE-MicroOS.x86_64-16.0.0-ContainerHost-OpenStack-Cloud-Snapshot20260207.qcow2",
ImageArchitecture: "amd64",
},
&api.Metadata{
// ... existing metadata fields ...
},
),
Entry("microos:1 aarch64", "aarch64", "testdata/microos-aarch64.SHA256SUM",
map[string]string{
common.DefaultInstancetypeEnv: "u1.medium",
common.DefaultPreferenceEnv: "opensuse.tumbleweed",
},
&api.ArtifactDetails{
Checksum: "REPLACE_WITH_CHECKSUM_FROM_microos-aarch64.SHA256SUM",
DownloadURL: "https://download.opensuse.org/tumbleweed/appliances/openSUSE-MicroOS.aarch64-16.0.0-ContainerHost-OpenStack-Cloud-Snapshot20260207.qcow2",
ImageArchitecture: "arm64",
},
&api.Metadata{
// Use the same metadata pattern as the x86_64 entry; adjust fields
// if the aarch64 image has different metadata (e.g., CreationDate).
},
),
```
You will also need to:
1. Add the fixture file `artifacts/opensuse/microos/testdata/microos-aarch64.SHA256SUM` with the correct SHA256 line for the aarch64 MicroOS image.
2. Replace `REPLACE_WITH_CHECKSUM_FROM_microos-aarch64.SHA256SUM` with the exact checksum parsed from that fixture.
3. If the actual aarch64 download URL differs (e.g. snapshot date or minor version), update the `DownloadURL` string to match the real image you expect the code to resolve.
This new `Entry` will force the tests to go through the `aarch64` branch of `retrieveBaseURL`, giving you full coverage of the base URL selection logic (x86_64, s390x, and aarch64).
</issue_to_address>
### Comment 2
<location> `artifacts/opensuse/microos/microos_test.go:18-19` </location>
<code_context>
+
+var _ = Describe("openSUSE MicroOS", func() {
+ DescribeTable("Inspect should be able to parse checksum files",
+ func(arch, mockFile string, envVariables map[string]string, details *api.ArtifactDetails, metadata *api.Metadata) {
+ c := New(arch, envVariables)
+ c.getter = testutil.NewMockGetter(mockFile)
+ got, err := c.Inspect()
</code_context>
<issue_to_address>
**issue (testing):** Update the New() call in tests to match the constructor signature and verify the sub-variant behavior
`New` is now defined as `(arch string, subvariant string, envVariables map[string]string)`, but the test calls it with only `(arch, envVariables)`, so this won’t compile. In addition to fixing the call, consider adding `subvariant` to the table data and asserting it is reflected in the resulting `DownloadURL`, so the default subvariant logic stays covered by tests.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
707ed2d to
c8b91cc
Compare
|
Hmmm, I seem to be missing something, according to golangci-lint: |
Signed-off-by: Johannes Kastl <[email protected]>
Signed-off-by: Johannes Kastl <[email protected]>
Signed-off-by: Johannes Kastl <[email protected]>
Signed-off-by: Johannes Kastl <[email protected]>
Signed-off-by: Johannes Kastl <[email protected]>
7632f61 to
757f465
Compare
| microos.New("x86_64", "ContainerHost-OpenStack-Cloud", defaultEnvVariables("u1.medium", "opensuse.tumbleweed")), | ||
| microos.New("s390x", "s390x-Cloud", defaultEnvVariables("u1.medium", "opensuse.tumbleweed")), |
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.
What's the difference in these flavors? Would the OpenStack-Cloud flavor not suffice as a minimal image? Does the s390x-Cloud flavor support cloud-init?
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 reason the flavors are named differently for x86_64 and s390x. And for the latter there does not seem to be images for both the Containerhost and the non-Containerhost flavor.
I cannot answer the last question, I just assume it is the same as for the tumbleweed image (as MicroOS is roughly "just an immutable Tumbleweed").
| <br /> | ||
| <br /> | ||
| Visit [get.opensuse.org/microos/](https://get.opensuse.org/microos/) to learn more about openSUSE MicroOS.` | ||
| const s390xArch = "s390x" |
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.
golangci-lint seems to complain about this line
artifacts/opensuse/microos/microos.go:33:1: File is not properly formatted (gofumpt)
const s390xArch = "s390x"
^
|
|
||
| var _ = Describe("openSUSE MicroOS", func() { | ||
| DescribeTable("Inspect should be able to parse checksum files", | ||
| func(arch, subvariant string, mockFile string, envVariables map[string]string, details *api.ArtifactDetails, metadata *api.Metadata) { |
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.
golangci-lint:
artifacts/opensuse/microos/microos_test.go:18:1: File is not properly formatted (gofumpt)
func(arch, subvariant string, mockFile string, envVariables map[string]string, details *api.ArtifactDetails, metadata *api.Metadata) {
^
What this PR does / why we need it:
This PR adds an image for openSUSE MicroOS.
Currently only one of the flavors is used, i.e. the Containerhost one which already has podman installed.