-
Notifications
You must be signed in to change notification settings - Fork 53
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
feat(24.04) add slice for crun and uidmap #225
feat(24.04) add slice for crun and uidmap #225
Conversation
Diff of dependencies: |
0bee945
to
2fc6357
Compare
2fc6357
to
450bfb9
Compare
450bfb9
to
a7162d3
Compare
a7162d3
to
0078313
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. There are some issues regarding the linting (see workflow runs) to be fixed, and it is suggested to add the copyright files explicitly in the slice definitions.
@cjdcordeiro @rebornplusplus can you approve the ci workflow please? |
fix yamllint errors
add copyright slice
sort essential
46eba25
to
239397e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for this @endersonmaia !
I think it's almost there. Apart from debconf
everything else is just trivial comments
slices/debconf.yaml
Outdated
contents: | ||
/usr/share/debconf/confmodule: | ||
/usr/share/debconf/confmodule.sh: | ||
/usr/share/debconf/debconf.conf: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this also a config file?
slices/debconf.yaml
Outdated
/usr/share/debconf/confmodule: | ||
/usr/share/debconf/confmodule.sh: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these aren't perl modules. the first is actually run during the maintainer scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it's ok to have them here, but I'd like to ask you to double check is 1) you really need them (cause if they are only used for the maintainer scripts, then they are not needed) and 2) if needed, try to reassess if they fit within this modules
slice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may note be the best person to check this, I have no knowledge in the packing internals.
slices/debconf.yaml
Outdated
/usr/share/debconf/confmodule.sh: | ||
/usr/share/debconf/debconf.conf: | ||
/usr/share/debconf/fix_db.pl: | ||
/usr/share/debconf/frontend: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although a script, this is also considered an interface configuration file, as it is used to configure the frontend. it is not a module. can u pls move it to another slice?
slices/debconf.yaml
Outdated
/usr/share/debconf/confmodule: | ||
/usr/share/debconf/confmodule.sh: | ||
/usr/share/debconf/debconf.conf: | ||
/usr/share/debconf/fix_db.pl: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a standalone script. I guess you might not always need it. I suggest moving it to a separate slice
slices/libpam0g.yaml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the debconf-2.0
dependency left out on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand they conflict, you'd have to choose one or the other, but again, I don't understand the debian packaging details, I'm just trying to build a small image and sharing what I've done upstream. 😓
Co-authored-by: Cristovao Cordeiro <[email protected]>
Could live without debconf for a chiselled environment? 😓 I mean, after you install all packages into a "chiselled" environment, you won't be using apt/dev anymore, right? |
That would be my gut feeling as well. I sympathize with the struggle here. My advice is: remove everything that is not needed for your use case to work. It's much easier to add additional slices and contents later. OTOH, we cannot remove |
I just need crun and uidmap for my use case, and it's working. I my last fixup I removed the debconf slice and dependency declaration to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes. If the tests pass, it all LGTM. Another approval (@rebornplusplus ) and I'll merge it ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thank you!
@rebornplusplus can you please approve again? I think you were re-requested by mistake |
Proposed changes
Forward porting
Testing
Checklist
Additional Context