-
Notifications
You must be signed in to change notification settings - Fork 87
Fixes #38938 - Add containerfile-install-command #1017
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
Conversation
chris1984
left a comment
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.
Have not tested, code looks good
qcjames53
left a comment
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.
Hey Pavan. This works pretty well! I have a few suggestions surrounding flavor but nothing that changes your core functionality here.
5aabdfd to
9bec932
Compare
|
I have made the suggested changes and pushed the updated code |
qcjames53
left a comment
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.
Your PR looks great to me. It works/fails correctly for many repos, no repos, missing hosts, and server errors. Approving.
While testing this, I also uncovered a bug with the API where only the first 20 repos are returned. I think this is my fault to be honest. I have full confidence your hammer command can handle 1000 repos no problem.
|
Looks good to me, will let @ianballou do a final look and merge |
ianballou
left a comment
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.
Small wording change to keep things consistent , looking good otherwise!
| end | ||
|
|
||
| class ContainerfileInstallCommand < HammerCLIKatello::Command | ||
| desc _("Generate a Containerfile RUN command from transiently installed packages on image-mode hosts") |
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.
| desc _("Generate a Containerfile RUN command from transiently installed packages on image-mode hosts") | |
| desc _("Generate a Containerfile RUN command from transiently installed packages on image mode hosts") |
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 have implemented the suggested changes.
9bec932 to
5838701
Compare
ianballou
left a comment
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!
Added a new Hammer CLI command to generate Containerfile RUN commands from a host's transiently installed packages for bootc/image-mode systems.