-
Notifications
You must be signed in to change notification settings - Fork 6
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
main: initial version of image-builder
with basic --list-images
#1
Conversation
417535f
to
59a4386
Compare
80b79f4
to
39060ad
Compare
ibuilder
with basic --list-images
image-builder
with basic --list-images
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.
Awesome! I put some "high-level" thoughts inline, but nothing is blocking (well, maybe the ibuilder
one). I guess we should convert them to issues/discussions at some point?
cmd/image-builder/main.go
Outdated
logrus.SetLevel(logrus.WarnLevel) | ||
|
||
rootCmd := &cobra.Command{ | ||
Use: "ibuilder", |
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 should be image-builder
SilenceUsage: true, | ||
} | ||
listImagesCmd.Flags().StringArray("filter", nil, `Filter distributions by a specific criteria (e.g. "type:rhel*")`) | ||
listImagesCmd.Flags().String("output", "", "Output in a specific format (text, json)") |
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 wonder if this should become an argument of rootCmd
in the future?
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.
A really good questions, probably yes - most verbs will have an --output switch (list-images, build). Not entirely sure about manifest though.
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.
It may be good to think of it as "console output". Manifests can be considered intermediary artifacts with a set format (JSON). The option could be to print them to stdout or save them into a file.
// that we keep this in sync | ||
// XXX2: means we need to depend on osbuild-composer-common or a new rpm | ||
// that provides the relevant packages *or* we use go:embed (cf images#1038) | ||
var repositoryConfigs = []string{ |
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.
Food for thought: Should we stop using our custom repo format, and start using the same format as dnf
uses in /etc/yum.repos.d
? Our depsolver should be already able to consume via the root
opion. Definitely not something to address in this PR, but I think it's a good conceptual dicusssion.
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 no strong opinion here, I would like to centralize what we have though. I would like to be able to build all supported imagetypes from this binary, ideally without much/any external dependencies. I was thinking go:embed for this and having the list of supported reos in images and the ability to override. But there is some pushback for this idea (not sure why though, I'm probably missing some details why this would be an issue).
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.
A dedicated RPM with repo definitions would make sense. We could split it as an osbuild-composer
sub-package for the beginning. WRT the format, I see little value in using the DNF format at the moment. Our repo definitions contain more information (such as "use RHSM?", image type tags, etc.).
This commit adds the new `image-builder` binary. This binary is meant to build images from the CLI without the need to setup a daemon. The main use-case is CI/CD and admins running this in scripts or ad-hoc. The CLI should be pleasant to use. This first commit adds the `list-images` command which is a thin wrapper around functionality from the `osbuild/images` library. It will list all buildable images by default and can be trimmed down further via `--filter` which supports the filtering from the `images` library, see osbuild/images#1015 It also supports `--output` which will output the result in the given format. Currently "text" and "json" are supported. Note that this will not work on it's own yet, it will need an installed image-builder to get the repositories. This will need to get fixed via either: 1. a dependency package for `ibuilder` that carries all the repos 2. a shared repo that contains the repos 3. using go:embed to get them (see images#1038)
This commit updates the GH workflow file to install the required dependencies to build the images library.
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 LGTM as the initial PR. 👍
The first commit still contains a mention of ibuilder
. I've added a few comments, but none of them should be considered blockers at this stage of development.
SilenceUsage: true, | ||
} | ||
listImagesCmd.Flags().StringArray("filter", nil, `Filter distributions by a specific criteria (e.g. "type:rhel*")`) | ||
listImagesCmd.Flags().String("output", "", "Output in a specific format (text, json)") |
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.
It may be good to think of it as "console output". Manifests can be considered intermediary artifacts with a set format (JSON). The option could be to print them to stdout or save them into a file.
// that we keep this in sync | ||
// XXX2: means we need to depend on osbuild-composer-common or a new rpm | ||
// that provides the relevant packages *or* we use go:embed (cf images#1038) | ||
var repositoryConfigs = []string{ |
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.
A dedicated RPM with repo definitions would make sense. We could split it as an osbuild-composer
sub-package for the beginning. WRT the format, I see little value in using the DNF format at the moment. Our repo definitions contain more information (such as "use RHSM?", image type tags, etc.).
This commit adds the new
image-builder
binary (name still a bit TBD but this is the best so far IMHO). This binary is meant to build images from the CLI without the need to setup a daemon. The main use-case is CI/CD and admins running this in scripts or ad-hoc. The CLI should be pleasant to use.This first commit adds the
list-images
command which is a thin wrapper around functionality from theosbuild/images
library.It will list all buildable images by default and can be trimmed down further via
--filter
which supports the filtering from theimages
library, see osbuild/images#1015It also supports
--output
which will output the result in the given format. Currently "text" and "json" are supported.Note that this will not work on it's own yet, it will need an installed image-builder to get the repositories. This will need to get fixed via either:
image-builder
that carries all the repos[edit: rename from ibuilder->image-builder after the most recent discussion during the standup, while there was no consensus it seems people did like image-builder a bit better]
Part of. COMPOSER-2395