Skip to content

libcontainer: move capabilities to separate package#2607

Merged
cyphar merged 1 commit into
opencontainers:masterfrom
thaJeztah:libcontainer_caps_refactor2
Feb 1, 2021
Merged

libcontainer: move capabilities to separate package#2607
cyphar merged 1 commit into
opencontainers:masterfrom
thaJeztah:libcontainer_caps_refactor2

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

I had these changes open in the branch I used for #2595. TBH, I don't exactly recall why I made these changes, but I guess because it's slightly cleaner to have it in a separate package (similar to cgroups, devices, apparmor, etc all being separate).

I'm not super-attached to it, so feel free to close if you don't like this change, but thought I'd push it to give the opportunity to review 😅

@kolyshkin @AkihiroSuda

@kolyshkin
Copy link
Copy Markdown
Contributor

While at it, we might start using internal to make sure no other package (except runc) starts using this package.

I am split between the top-level internal and libcontainer/internal. In this very case I'm in favor of the latter.

@kolyshkin
Copy link
Copy Markdown
Contributor

internal is already used in #2569

@kolyshkin
Copy link
Copy Markdown
Contributor

I don't exactly recall why I made these changes

Probably because of func init() in libcontainer/capabilities_linux.go, so it won't be always run.

I just found myself creating an alternative to this one: #2646

@thaJeztah
Copy link
Copy Markdown
Member Author

Ah, yes, avoiding init is definitely good. I still think that having it in a separate package is clean (not critical of course); slightly on the fence on using "internal" (which would hard restrict external users); guess "libcontainer" is now a bit of a confusing name, as it's no longer meant to use as a library (by others)

@AkihiroSuda
Copy link
Copy Markdown
Member

What's current status?

crosbymichael
crosbymichael previously approved these changes Nov 10, 2020
AkihiroSuda
AkihiroSuda previously approved these changes Nov 13, 2020
@thaJeztah
Copy link
Copy Markdown
Member Author

@kolyshkin good to go?

@AkihiroSuda
Copy link
Copy Markdown
Member

Could you rebase to retrigger CI?

@thaJeztah thaJeztah dismissed stale reviews from AkihiroSuda and crosbymichael via e9a3e4b December 4, 2020 10:56
@thaJeztah thaJeztah force-pushed the libcontainer_caps_refactor2 branch from 007ecf7 to e9a3e4b Compare December 4, 2020 10:56
@thaJeztah
Copy link
Copy Markdown
Member Author

rebased 👍

AkihiroSuda
AkihiroSuda previously approved these changes Dec 5, 2020
@thaJeztah
Copy link
Copy Markdown
Member Author

@kolyshkin @crosbymichael PTAL; this one good to go?

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

rebased.

@AkihiroSuda @kolyshkin @crosbymichael PTAL; this one good to go?

@AkihiroSuda
Copy link
Copy Markdown
Member

@cyphar @kolyshkin Can we have this in rc93?

Copy link
Copy Markdown
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM. Regarding internal I think we should move basically all of libcontainer into internal at some point and move the bits of code people actually use to be outside internal, so that the default state of libcontainer is private.

@cyphar cyphar closed this in 091dd32 Feb 1, 2021
@cyphar cyphar merged commit 091dd32 into opencontainers:master Feb 1, 2021
@thaJeztah thaJeztah deleted the libcontainer_caps_refactor2 branch February 1, 2021 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants