[release/1.7] Migrate platforms package to github.com/containerd/platforms #10292
Conversation
|
Skipping CI for Draft Pull Request. |
|
validate vendor fails, because |
026143a to
76e75ee
Compare
|
Got some failure on Windows that may be related; restarted them just in case it was flaky, but ... it's possible there's some backward-incompatible changes in the |
|
Same failures again on Windows, but not sure immediately what the error is, or more accurately; how the error relates to this PR :thinking_face: https://github.com/containerd/containerd/actions/runs/9351458248/job/25741492452?pr=10292 |
platforms/platforms_deprecated.go
Outdated
| // | ||
| // Deprecated: use [platforms.DefaultString]. | ||
| func DefaultString() string { | ||
| return platforms.DefaultString() |
There was a problem hiding this comment.
Can you try to keep this as return Format(DefaultSpec()), platforms DefaultString() is now using return FormatAll(DefaultSpec()) which includes the OS version.
There was a problem hiding this comment.
thx; can give that a try; I had a quick peek and it looks like that's only used in 1 place (in ctr to set the default value).
Perhaps that one should use the underlying code instead 🤔
There was a problem hiding this comment.
oh, never mind, looking in the wrong place (imgcrypt). Let me look further.
There was a problem hiding this comment.
I pushed a commit, but perhaps we should indeed consider to make this a separate function after all (new function to return default including os-version, and keep the old one without 🤔)
|
Unfortunately looks like that didn't help 🤔 |
de1d82c to
187adf6
Compare
|
The failure is from no matching differ, I added a log to see which match was failing The target platform doesn't have os version and does not match Windows with an os version... |
|
Thanks! So could indeed be code that's depending on "Parse" to fill in the system's version perhaps. Also.. not near my computer, but now I want to open a PR to find where that grammar error is ("do to" -> "due to") 🤣🤣 |
|
Thats just from a debug commit of mine, I'll track it down. Oddly the matcher seems to pass in that situation today in unit testing. |
|
Opened containerd/platforms#11 to fix |
187adf6 to
df7298b
Compare
|
I opened a draft PR on main to update the platforms package to current main; And I added a cherry-pick for that here |
| func WithPlatform(platform string) RemoteOpt { | ||
| if platform == "" { | ||
| platform = platforms.DefaultString() | ||
| platform = platforms.Format(platforms.DefaultSpec()) // For 1.7 continue using the old format without os-version included. |
There was a problem hiding this comment.
If CI is happy with the updated platforms package, I'll also see if it's still happy if I would revert these changes
df7298b to
3732e8d
Compare
|
CI was happy; now reverting one commit to see if it was actually needed |
f3e8bc0 to
f6a7b49
Compare
|
ok; it fails with that commit reverted, so looks like we need to keep it indeed |
0b32be5 to
1bd1ee1
Compare
This updates the platforms package to be an alias for the new platforms module. This helps transitioning consumers to the new module, and makes sure that containerd v2 and v1 use the same definitions. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The platforms.DefaultString() function in the new module returns a string including the os-version. The existing code does not expect that to be the case, so using the previous format instead. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- Remove hcsshim import from repo
- un-exports GetOsVersion
- Update windows matcher to not compare empty os version
full diff: containerd/platforms@v0.2.0...v0.2.1
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 87dd430)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
1bd1ee1 to
869b786
Compare
|
/retest |
|
xref: #9662 |
Also include partial backports / patches similar to;
migrate platforms package to github.com/containerd/platforms
This updates the platforms package to be an alias for the new platforms module.
This helps transitioning consumers to the new module, and makes sure that
containerd v2 and v1 use the same definitions.
platforms: mark aliases as deprecated