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

Cleanup plugin interface #2

Merged
merged 2 commits into from
Oct 25, 2023

Conversation

dmcgowan
Copy link
Member

Separate Get for set and init context, which are used differently.
Ensure init context always returns instances and does not return an underlying map.

Update these interfaces before tagging v0.1.0 release

Related to containerd/containerd#9258

Separate Get for set and init context, which are used differently.
Ensure init context always returns instances and does not return an
underlying map.

Signed-off-by: Derek McGowan <[email protected]>
context.go Outdated Show resolved Hide resolved
Remove the `Get` which just returns the first instance while ignoring
other instances. GetSingle will ensure that there is only a single
instance of a plugin and allow dependencies to enforce it or have logic
to select the right plugin.

Signed-off-by: Derek McGowan <[email protected]>
@dmcgowan
Copy link
Member Author

@samuelkarp Added a commit which changes the interface. I think this might be safer in the long run to enforce getting a single instance or selecting the right instance. Just getting the first instance from an unordered map is the kind of undefined behavior we want to avoid.

Copy link
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

I like this behavior much better.

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

are we safe to just remove the initcontext get by type? If not sure.. could just leave it in, deprecate it and proclaim a warning loudly..

@samuelkarp
Copy link
Member

are we safe to just remove the initcontext get by type? If not sure.. could just leave it in, deprecate it and proclaim a warning loudly..

This is a new module with a new import path, so users already will need to opt-in to use it. I think this is a reasonable change to make before we make a first release.

@dmcgowan
Copy link
Member Author

We kind of have a one time chance when switching import path/modules. Although some packages stay under 1.0 in libraries to make it easier to change. I think in practice we won't need to make any other changes anytime soon for this part of the interface.

@dmcgowan dmcgowan merged commit e13be35 into containerd:main Oct 25, 2023
4 checks passed
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.

3 participants