fix: remove additional tags from image URL names#4928
fix: remove additional tags from image URL names#4928Mysterio-17 wants to merge 1 commit intolima-vm:masterfrom
Conversation
|
Hello @unsuman @jandubois , The |
jandubois
left a comment
There was a problem hiding this comment.
Please remove the dummy commit to trigger CI from the branch.
If CI failure looks like it cannot be a result of the PR change, please wait for a maintainer to rerun the tests. Unfortunately some tests are quite flakey, so this is needed more often than we like.
| t.Run("deduplicates arch aliases", func(t *testing.T) { | ||
| name := limatmpl.InstNameFromImageURL("FreeBSD-15.0-RELEASE-arm64-aarch64-BASIC-CLOUDINIT-zfs.raw.xz", limatype.AARCH64) | ||
| if limatype.IsNativeArch(limatype.AARCH64) { | ||
| assert.Equal(t, name, "freebsd-15.0-zfs") | ||
| } else { | ||
| assert.Equal(t, name, "freebsd-15.0-arm64-zfs") | ||
| } | ||
| }) |
There was a problem hiding this comment.
This test only tests the alternate native arch removal on the aarch64. It would be better to construct a filename with an alias in it to begin with. Something like
alias = runtime.GOARCH
for keyword, arch := range archKeywords {
if limatype.IsNativeArch(arch) && arch != keyword {
alias = keyword
break
}
}
image := fmt.Sprintf("...-%s-basic", alias)| { | ||
| locator: "http://ftp-archive.freebsd.org/pub/FreeBSD-Archive/old-releases/VM-IMAGES/15.0-RELEASE/amd64/Latest/FreeBSD-15.0-RELEASE-amd64-BASIC-CLOUDINIT-zfs.raw.xz", | ||
| expectedName: "freebsd-15.0-amd64-zfs", | ||
| nativeName: "freebsd-15.0-zfs", |
There was a problem hiding this comment.
These tests seem redundant with the test above? How can they fail when the test above passes?
There was a problem hiding this comment.
These tests seem redundant with the test above? How can they fail when the test above passes?
Yeah... TestRead calls InstNameFromImageURL() internally, so these can't fail independently. I'll drop the nativeName field and the new TestRead entry, keeping only the direct TestInstNameFromImageURL test.
1785224 to
607aa0d
Compare
|
@jandubois , I've made the changes, rewrote the test to dynamically use the native arch alias so it works on any host, removed the redundant name checks from |
|
I only see that you dropped the empty commit. I don't see any other changes. |
607aa0d to
afb350b
Compare
|
Hi @jandubois , I've made the changes, Kindly review it |
| alias := runtime.GOARCH | ||
| if arch == alias { | ||
| t.Skip("no alias for native arch") | ||
| } | ||
| image := fmt.Sprintf("linux-%s.raw", alias) | ||
| name := limatmpl.InstNameFromImageURL(image, arch) |
There was a problem hiding this comment.
This version does not always pick an alias; I've given you a better version already in my previous review. Here is a refined variant:
| alias := runtime.GOARCH | |
| if arch == alias { | |
| t.Skip("no alias for native arch") | |
| } | |
| image := fmt.Sprintf("linux-%s.raw", alias) | |
| name := limatmpl.InstNameFromImageURL(image, arch) | |
| var alias string | |
| for keyword, arch := range archKeywords { | |
| if limatype.IsNativeArch(arch) && arch != keyword { | |
| alias = keyword | |
| break | |
| } | |
| } | |
| if alias == "" { | |
| t.Skip("no alias for native arch") | |
| } | |
| image := fmt.Sprintf("linux-%s-basic.raw", alias) | |
| name := limatmpl.InstNameFromImageURL(image, limatype.NewArch(runtime.GOARCH)) |
This performs the test even when runtime.GOARCH is arm or ppc64le, which in your test would just skip.
| t.Run(tt.locator, func(t *testing.T) { | ||
| tmpl, err := limatmpl.Read(t.Context(), "", tt.locator) | ||
| assert.NilError(t, err) | ||
| assert.Equal(t, tmpl.Name, tt.expectedName) |
There was a problem hiding this comment.
I don't know why you are removing this test; it needs to stay.
There was a problem hiding this comment.
I'm only talking about the expectedName test. It existed before your change, and needs to stay. The additional nativeName test you added is redundant with the earlier test and should be removed.
afb350b to
858c2ef
Compare
|
Hey @jandubois , I've implemented your suggested |
| t.Run(tt.locator, func(t *testing.T) { | ||
| tmpl, err := limatmpl.Read(t.Context(), "", tt.locator) | ||
| assert.NilError(t, err) | ||
| assert.Equal(t, tmpl.Name, tt.expectedName) |
There was a problem hiding this comment.
I'm only talking about the expectedName test. It existed before your change, and needs to stay. The additional nativeName test you added is redundant with the earlier test and should be removed.
When the image arch matches the native arch, remove all arch keyword aliases (e.g. arm64, amd64) in addition to the canonical arch name (e.g. aarch64, x86_64). Previously only the canonical name was removed, leaving aliases like arm64 in the instance name. For example, on an aarch64 host: FreeBSD-15.0-RELEASE-arm64-aarch64-BASIC-CLOUDINIT-zfs.raw.xz Before: freebsd-15.0-arm64-zfs After: freebsd-15.0-zfs Signed-off-by: Mysterio-17 <mradultiwari1708@gmail.com>
858c2ef to
2bd265a
Compare
|
Thanks for the review! I've updated the alias test to use your suggested loop across archKeywords so it works properly on all architectures. I also removed nativeName entirely and restored all the original expectedName values, updating the test to dynamically check for the stripped arch name inside the test assertion block instead. |
Description
When deriving an instance name from an image URL, arch aliases (e.g. arm64 for aarch64, amd64 for x86_64) were not being removed even when the image arch matched the native host arch. Only the canonical arch name was stripped.
Changes
pkg/limatmpl/locator.go: When the image arch is native, iterate over all archKeywords entries and remove any keyword that maps to the same arch, not just the canonical name.pkg/limatmpl/locator_test.go: Added test for arch alias deduplication, added the FreeBSD 15.0 aarch64 URL from the issue to TestRead, and made arch-dependent tests use a nativeName field so they pass on any host architecture.Fixes: #4629