Skip to content

pkg/system: deprecate IsOSSupported() and ErrNotSupportedOperatingSystem, and implement image.CheckOS#46416

Merged
thaJeztah merged 2 commits intomoby:masterfrom
thaJeztah:move_IsOSSupported
Sep 7, 2023
Merged

pkg/system: deprecate IsOSSupported() and ErrNotSupportedOperatingSystem, and implement image.CheckOS#46416
thaJeztah merged 2 commits intomoby:masterfrom
thaJeztah:move_IsOSSupported

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Sep 6, 2023 •
edited
Loading

We should still look for all places where this is used, as there may be quite some that are either not needed, or (with multi-arch image support) potentially unwanted, but let's at least slightly improve things;

  • pkg/system: move ErrNotSupportedOperatingSystem to where it's used
  • pkg/system: move IsOSSupported to "image" package
    This function is used in combination with images, so move it there to prevent having to import pkg/system (which contains many unrelated functions)
  • image: make ErrNotSupportedOperatingSystem an errdefs.ErrInvalidParameter
  • image: implement CheckOS
    Implement a function that returns an error to replace existing uses of the IsOSSupported utility, where callers had to produce the error after checking.

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah added status/2-code-review area/images Image Service kind/refactor PR's that refactor, or clean-up code labels Sep 6, 2023
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Sep 6, 2023

Ah, dang! Forgot about this one (think I had a similar branch somewhere);

#23 1.812 package github.com/docker/docker/cmd/dockerd
#23 1.812 	imports github.com/docker/docker/api/server/backend/build
#23 1.812 	imports github.com/docker/docker/builder
#23 1.812 	imports github.com/docker/docker/container
#23 1.812 	imports github.com/docker/docker/image
#23 1.812 	imports github.com/docker/docker/layer
#23 1.812 	imports github.com/docker/docker/daemon/graphdriver
#23 1.812 	imports github.com/docker/docker/pkg/archive
#23 1.812 	imports github.com/docker/docker/pkg/system
#23 1.812 	imports github.com/docker/docker/image: import cycle not allowed

I guess we can't provide the "deprecated" aliases for this one.

@thaJeztah thaJeztah force-pushed the move_IsOSSupported branch 2 times, most recently from 536f700 to ee48122 Compare September 6, 2023 21:16
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Sep 7, 2023

Flaky test on Windows;

=== FAIL: github.com/docker/docker/integration/container TestResizeWhenContainerNotStarted (18.69s)
    resize_test.go:58: timeout hit after 10s: waiting for container to be one of (exited), currently running

@thaJeztah thaJeztah changed the title pkg/system: deprecate system.IsOSSupported, and implement image.CheckOS pkg/system: remove system.IsOSSupported, and implement image.CheckOS Sep 7, 2023
@thaJeztah thaJeztah changed the title pkg/system: remove system.IsOSSupported, and implement image.CheckOS pkg/system: remove IsOSSupported() and ErrNotSupportedOperatingSystem, and implement image.CheckOS Sep 7, 2023
@thaJeztah thaJeztah marked this pull request as ready for review September 7, 2023 12:03
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Sep 7, 2023

I should mention that this PR should not be considered be an “endorsement” of IsOSSupported (now CheckOS), because we likely must get rid of most uses of it, but my intent here is to keep pkg/system out of parts where it should not be depended on, and keep this functionality close to what’s it for (image)

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Sep 7, 2023

discussed in the maintainers call, and the suggestion was to, instead of an "alias", just keep a temporary ("deprecated") copy of the original function (and error) in pkg/system so that (if there are external consumers) the transition will be slightly smoother.

@thaJeztah
pkg/system: move ErrNotSupportedOperatingSystem to where it's used
150b657 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
image: implement CheckOS, deprecate pkg/system IsOSSupported
a3c97be 
Implement a function that returns an error to replace existing uses of
the IsOSSupported utility, where callers had to produce the error after
checking.

The IsOSSupported function was used in combination with images, so implementing
a utility in "image" to prevent having to import pkg/system (which contains many
unrelated functions)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah changed the title pkg/system: remove IsOSSupported() and ErrNotSupportedOperatingSystem, and implement image.CheckOS pkg/system: deprecate IsOSSupported() and ErrNotSupportedOperatingSystem, and implement image.CheckOS Sep 7, 2023
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Sep 7, 2023

alright; done 👍

@corhere @neersighted PTAL

@thaJeztah thaJeztah merged commit 06499c5 into moby:master Sep 7, 2023
@thaJeztah thaJeztah deleted the move_IsOSSupported branch September 7, 2023 22:25
@thaJeztah thaJeztah added this to the 25.0.0 milestone Oct 26, 2023
@thaJeztah thaJeztah mentioned this pull request Jan 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rumpl rumpl rumpl approved these changes

@neersighted neersighted neersighted approved these changes

@tonistiigi tonistiigi Awaiting requested review from tonistiigi tonistiigi is a code owner

Assignees

No one assigned

Labels

area/images Image Service kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Milestone

25.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@thaJeztah @rumpl @neersighted