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

Rename the rootless struct to UserNamespaceConfig #2257

Merged
merged 1 commit into from Aug 14, 2023

Conversation

YJDoc2
Copy link
Collaborator

@YJDoc2 YJDoc2 commented Aug 12, 2023

The current Rootless struct has more to do with creating a new user namespace that rootless containers. For example, it is possible and valid that a root user will run a container with new user ns, and in such case the current code treats it as "running in rootless" . This PR simply re-names stuff, but does not actually change any functionality. This is roughly 1st of 3 parts for making podman rootless work.

The main changes here are changing the rootless.rs file to user_ns.rs , and changing the struct names

  • Rootless -> UserNamespaceConfig
  • RootlessIDMapper -> UserNamespaceIDMapper
  • RootlessError -> UserNamespaceError

Few of the struct members are also renamed to reflect these. Apart from this file, changes in other files are simply to update for these re-naming. If you have any better idea for the names, feel free to comment.

@YJDoc2 YJDoc2 added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Aug 12, 2023
@YJDoc2 YJDoc2 requested a review from a team August 12, 2023 14:07
@YJDoc2 YJDoc2 self-assigned this Aug 12, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2023

Codecov Report

Merging #2257 (4e26799) into main (e5232ce) will decrease coverage by 0.01%.
Report is 2 commits behind head on main.
The diff coverage is 63.41%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2257      +/-   ##
==========================================
- Coverage   65.00%   65.00%   -0.01%     
==========================================
  Files         129      129              
  Lines       15149    15153       +4     
==========================================
+ Hits         9848     9850       +2     
- Misses       5301     5303       +2     

Copy link
Member

Choose a reason for hiding this comment

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

Could you create an issue for releasing v0.2.0 instead of this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey, I created this file, as a lot of libraries when releasing breaking versions usually have some info on what has changed and what changes wold need to be done for upgrading to new version (separate from release notes). That way anyone upgrading can see that and make changes accordingly.

I can make an issue for the release, but do you still want to remove this file?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your response!

We are responsible for not only libcontainer but also youki itself. I'm worried that this filename makes youki's user confusing.

Hey, I created this file, as a lot of libraries when releasing breaking versions usually have some info on what has changed

I want to know why we need to separate the release note and this file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair enough, It can be confusing for youki users, as there is no upgrade for youki as a whole, only lib*

I'm worried that this filename makes youki's user confusing.

Usually from what I have seen for other libraries, in release notes, they usually link PRs and important changes, and then have a separate .MD file, or a section in readme which lists what changes in code users of the library will need to do in order to upgrade to new version. I thought we should have something similar for lib* . So for example, the release note will only contain Changed executor interface for composibility and ease-of-use , whereas the other file will list steps one might take or considerations one might have for changing their own executor implementation. It will be a more detailed and longer description than release notes. Currently I have only listed structure re-names, as that is only changed in this PR.

I want to know why we need to separate the release note and this file.

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

👍

@utam0k utam0k merged commit 539885f into containers:main Aug 14, 2023
13 checks passed
@YJDoc2 YJDoc2 deleted the rename-rootless-struct branch January 2, 2024 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants