Skip to content

Move RunningInUserNS() to its own package#5252

Merged
estesp merged 1 commit intocontainerd:masterfrom
thaJeztah:separate_userns
Mar 23, 2021
Merged

Move RunningInUserNS() to its own package#5252
estesp merged 1 commit intocontainerd:masterfrom
thaJeztah:separate_userns

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Mar 23, 2021

Trying to prevent the sys package to become too much of a kitchen-sink 😅

This allows using the utility without bringing whole of "sys" with it.

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Mar 23, 2021

@AkihiroSuda @estesp ptal 🤗

AkihiroSuda
AkihiroSuda reviewed Mar 23, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe we want to add RunningInUserNS = userns.RunningInUserNS alias in sys to reduce vendoring issues

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Mar 23, 2021

maybe we want to add RunningInUserNS = userns.RunningInUserNS alias in sys to reduce vendoring issues

I recall a discussion around sys not intended for external use (so no backward compatibility); that said; happy to add an alias; let me add one, and we can decide if we keep that or not

@thaJeztah
Move RunningInUserNS() to its own package
708299c 
This allows using the utility without bringing whole of "sys" with it.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Mar 23, 2021

Updated and added an alias

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Mar 23, 2021

I recall a discussion around sys not intended for external use (so no backward compatibility)

I guess that discussion was about libcontainer/system?
runc (libcontainer) is not really intended to be used as a library today, but containerd is still intended to be used as a library.

Anyway, we don't strictly guarantee compatibility of Go API, so this LGTM.

dims
dims approved these changes Mar 23, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dims dims left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Mar 23, 2021

I guess that discussion was about libcontainer/system?

No, it was about containerd/sys (don't recall the details, but I think it was more that sys was mostly an "internal" utility package). The alias is just a "one line" thing (well, boiler plating is more than the actual code 😂), so I'm 👍 for having the alias to ease the transition.

estesp
estesp approved these changes Mar 23, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 717dde3 into containerd:master Mar 23, 2021
@thaJeztah thaJeztah deleted the separate_userns branch March 23, 2021 12:59
@thaJeztah thaJeztah mentioned this pull request Mar 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dims dims dims approved these changes

@estesp estesp estesp approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@thaJeztah @AkihiroSuda @dims @estesp