Skip to content
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

platforms: isolate from errdefs and api dependencies #9095

Merged
merged 2 commits into from Sep 14, 2023

Conversation

thaJeztah
Copy link
Member

platforms: remove errdefs dependency

This cleans up the platforms package from dependencies that are not strictly
needed. This is in preparation of making this package a separate module, which
can be shared by plugins, and containerd versions (as well as external consumers),

  • Remove dependency on the errdefs package: most uses of these error
    definitions were used internally, and other errors may not be useful
    for external consumers as sentinel errors.
  • ErrInvalidArgument may be a potential exception, although a look at
    current uses of this package shows that there's no special handling of
    invalid parameters vs other errors (all would boil down to "the passed
    platform is invalid" (either the format, or parsing is not implemented
    on a specific platform)
  • Remove uses of the convenience "Platform" alias in favor of using the
    upstream (from OCI spec). Consumers of this package can still use the
    convenience alias, but make sure that function signatures do not imply
    that it's a different type (which can cause confusion).

platforms: move ToProto, FromProto to api/types

These utilities resulted in the platforms package to have the containerd
API as dependency. As this package is used in many parts of the code, as
well as external consumers, we should try to keep it light on dependencies,
with the potential to make it a standalone module.

These utilities were added in f3b7436,
which has not yet been included in a release, so skipping deprecation
and aliases for these.

This cleans up the platforms package from dependencies that are not strictly
needed. This is in preparation of making this package a separate module, which
can be shared by plugins, and containerd versions (as well as external consumers),

- Remove dependency on the errdefs package: most uses of these error
  definitions were used internally, and other errors may not be useful
  for external consumers as sentinel errors.
- ErrInvalidArgument may be a potential exception, although a look at
  current uses of this package shows that there's no special handling of
  invalid parameters vs other errors (all would boil down to "the passed
  platform is invalid" (either the format, or parsing is not implemented
  on a specific platform)
- Remove uses of the convenience "Platform" alias in favor of using the
  upstream (from OCI spec). Consumers of this package can still use the
  convenience alias, but make sure that function signatures do not imply
  that it's a different type (which can cause confusion).

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These utilities resulted in the platforms package to have the containerd
API as dependency. As this package is used in many parts of the code, as
well as external consumers, we should try to keep it light on dependencies,
with the potential to make it a standalone module.

These utilities were added in f3b7436,
which has not yet been included in a release, so skipping deprecation
and aliases for these.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Copy link
Member

@henry118 henry118 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@dcantah dcantah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice LGTM

@estesp estesp merged commit 3f315fc into containerd:main Sep 14, 2023
45 checks passed
@thaJeztah thaJeztah deleted the isolate_platform branch September 14, 2023 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants