MCO-2163: adds machine-config-osimagestream installer helper#5770
Conversation
WalkthroughAdds a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
approved
cheesesashimi
force-pushed
the
zzlotnik/add-osimagestream-generation-cmd
branch
from
7904d0a to
a479290
Compare
| getCmd.PersistentFlags().StringVar(&o.outputFormat, "output-format", "json", "The output format to use: 'json' or 'yaml'.") | ||
| getCmd.PersistentFlags().StringVar(&o.outputFile, "output-file", "", "Path to write output to a file instead of stdout. Format is auto-detected from extension (.json, .yaml, .yml).") | ||
|
|
||
| getCmd.MarkPersistentFlagRequired("pull-secret") |
There was a problem hiding this comment.
While this seems good to me, I think most of the bootstrap code in the installer is thought to use default paths. The installer generated ignition for the bootstrap node takes care of placing the pull-secret, mirrot and proxy configuration in the correct file so calls to podman (or skopeo) don't need to provide those args everywhere. The only exception I've observed is rpm-ostree, that requiers the pull-secret to be passed with --authfile.
Following to the previous point, we need to make really sure mirror, proxy and custom CAs are properly loaded, no matter if it's because the defaults of container-libs do the job or because we use the builder imageutils.NewSysContextBuilder returns to craft a proper auth/tls/mirror/proxy context.
There was a problem hiding this comment.
I've done a quick search in the installer and these are my findings:
- I think the proxy env vars are automatically loaded by ignition into systemd.conf.d/10-default-env.conf and into /etc/profile.d/proxy.sh. I'd expect that the proxy env vars are present when the unit runs.
- registries.conf (mirror rules): I've seen a few places (specially in the agent, that is the workflow that relies on them, but not exclusively) where the installer generates it for the final ignition. Not too sure about this one, but my vet is that the file should be already placed in
/etc/containers/registries.confwhen the installer units run. If not, I cannot explain why rpm-ostree works. - pull-secret. Exists cause it's rendered by this template file https://github.com/openshift/installer/blob/791a41c20c5d3c72dee4aca31df817710a7d39b0/data/data/bootstrap/files/root/.docker/config.json.template (everything inside the files directory of the assets it pushed to the bootstrap by ignition)
- Custom CAs: Not 100% sure, but seems to be like the previous point: https://github.com/openshift/installer/blob/59e504062d51674fc20394ac043bf0e3e022c385/data/data/bootstrap/files/etc/pki/ca-trust/source/anchors/ca.crt.template
There was a problem hiding this comment.
As I began integrating this into the installer a bit (see: openshift/installer#10406), I began to realize that another problem we'll have to deal with is mounting things into the container this will run in. I contemplated an operational mode where we extract the binary from the image and run it natively on the bootstrap host so that it can access what it needs given the original execution context.
cheesesashimi
force-pushed
the
zzlotnik/add-osimagestream-generation-cmd
branch
4 times, most recently
from
99a50d4 to
0de6a09
Compare
cheesesashimi
force-pushed
the
zzlotnik/add-osimagestream-generation-cmd
branch
3 times, most recently
from
cddae24 to
50150a3
Compare
|
|
||
| // Wraps imageRegistrySecretImpl and implements UnmarshalJSON so that | ||
| // progressive JSON decoding may be used. | ||
| type dockerConfigJSONDecoder struct { |
There was a problem hiding this comment.
This implementation would need some UT testing. I know the type it's not exported so we may not want to directly test it, but test the caller with some inputs that give us good coverage of this piede of code.
There was a problem hiding this comment.
I decided to drop this change from this PR. I ran into what I thought was a bug when I was setting up some test cases, but it turns out that I was not setting up my test cases correctly. Instead, I will solely add some additional test cases to pkg/secret to codify what I was doing wrong without any other code changes in that package.
| CertDir: opts.certsPath, | ||
| PerHostCertDir: opts.perHostCertsPath, | ||
| PullSecret: opts.pullSecretPath, | ||
| RegistryConfig: opts.registryConfigPath, |
There was a problem hiding this comment.
Question: Should we provide safe defaults and always pass them to container-libs or are we advocating to the container-libs defaults?
In other words, do we want to pick a default and never trust/rely on their defaults?
There was a problem hiding this comment.
I could go either way with this, honestly.
| # Get the full image pullspec from release image: | ||
| $ machine-config-osimagestream get default-node-image \ | ||
| --pull-secret /path/to/pull/secret \ | ||
| --release-image quay.io/openshift-release-dev/ocp-release:latest \ | ||
| --image-pullspec | ||
|
|
||
| # Get the image tag name from ImageStream file: | ||
| $ machine-config-osimagestream get default-node-image \ | ||
| --pull-secret /path/to/pull/secret \ | ||
| --imagestream /path/to/imagestream.json \ | ||
| --image-name | ||
|
|
||
| # Get the full image pullspec from ImageStream file: | ||
| $ machine-config-osimagestream get default-node-image \ | ||
| --pull-secret /path/to/pull/secret \ | ||
| --imagestream /path/to/imagestream.yaml \ | ||
| --image-pullspec | ||
|
|
||
| # With per-host certificates (output tag name): | ||
| $ machine-config-osimagestream get default-node-image \ | ||
| --pull-secret /path/to/pull/secret \ | ||
| --per-host-certs /etc/containers/certs.d \ | ||
| --release-image quay.io/openshift-release-dev/ocp-release:latest \ | ||
| --image-name | ||
|
|
||
| # With custom certificates and registry configuration (output pullspec): | ||
| $ machine-config-osimagestream get default-node-image \ | ||
| --pull-secret /path/to/pull/secret \ | ||
| --certs /path/to/certs \ | ||
| --registry-config /etc/containers/registries.conf \ | ||
| --release-image quay.io/openshift-release-dev/ocp-release:latest \ | ||
| --image-pullspec | ||
|
|
||
| # With additional trust bundle (single file, output tag name): | ||
| $ machine-config-osimagestream get default-node-image \ | ||
| --pull-secret /path/to/pull/secret \ | ||
| --trust-bundle /etc/pki/ca-trust/source/anchors/ca-bundle.crt \ | ||
| --release-image quay.io/openshift-release-dev/ocp-release:latest \ | ||
| --image-name | ||
|
|
||
| # With multiple trust bundles (files and directories, output pullspec): | ||
| $ machine-config-osimagestream get default-node-image \ | ||
| --pull-secret /path/to/pull/secret \ | ||
| --trust-bundle /etc/pki/ca-trust/source/anchors/ \ | ||
| --trust-bundle /opt/custom/cert.pem \ | ||
| --release-image quay.io/openshift-release-dev/ocp-release:latest \ | ||
| --image-pullspec`, |
There was a problem hiding this comment.
I've the feeling this is way too much for the help option of the binary. I'd say the examples are nice, but I see them fitting better in a doc md.
The same applies to 55c004f#diff-ad14b191f88ac54c22961ae04f78ba66ecadad7b167abeae58a377ef3d0d85c4R38
There was a problem hiding this comment.
Yeah, I agree. I'll rework the examples and produce a README file instead.
| getCmd.PersistentFlags().StringVar(&o.releaseImage, "release-image", "", "The OCP / OKD release payload image to run against.") | ||
| getCmd.PersistentFlags().StringVar(&o.imageStreamPath, "imagestream", "", "Path to the ImageStream file to run against.") | ||
|
|
||
| getCmd.MarkPersistentFlagRequired("pull-secret") |
There was a problem hiding this comment.
I think most tools tools use --authfile to provide a pull-secret, and if not given they fallback to ${XDG_RUNTIME_DIR}/containers/auth.json (among others, including the one the installer uses for everything, including podman /root/.docker./config.json https://github.com/containers/skopeo/blob/f9d4a40261d9f6dd924d22274805cd075e1fe939/vendor/go.podman.io/common/pkg/auth/auth.go#L83.
There was a problem hiding this comment.
Good call. I will change that.
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/ghodss/yaml" |
There was a problem hiding this comment.
"sigs.k8s.io/yaml", that uses github.com/ghodss/yaml underneath, is a bit better fit I think:
- It's maintained
- It handles multline strings better. (not required for this use case)
There was a problem hiding this comment.
Agreed. I keep forgetting about that library. Will change my code to use that.
|
|
||
| // TODO: Not sure if this value is used by types.SystemContext or not. But | ||
| // we'll retrieve it anyway for right now. | ||
| if noProxy := os.Getenv("NO_PROXY"); noProxy != "" { |
There was a problem hiding this comment.
Unfortunately no, I introduced a change in container-libs to be able to properly consume the 3 fields https://github.com/containers/container-libs/pull/583/changes (it will allow us to provide a callback that consumes them), but given that they don't backport and we cannot upgrade we cannot use it.
There was a problem hiding this comment.
I'll remove it for now since it's going unused. But I'll leave a comment in its place containing this context.
| } | ||
|
|
||
| return getImageStreamFromReleaseImage(ctx, sysCtx, releaseImage) | ||
| } |
There was a problem hiding this comment.
I'm a bit surprissed about the content of this function, as I'd expected something more like this:
func getImageStreamProvider(ctx context.Context, imagesInspector ImagesInspector, imageStreamPath, releaseImage string) (osimagestream.ImageStreamProvider, error)
if imageStreamPath != "" {
is, err := getImageStreamFromFile(imageStreamPath)
if err != nil {
return nil, err
}
return osimagestreams.NewImageStreamProviderResource(is)
}
return osimagestreams.NewImageStreamProviderNetwork(imagesInspector, releaseImage)
}Later on you can directly pass the ImageStreamProvider to the NewImageStreamStreamSource you already have in place. The ImagesInspector you create later would be now created a bit before to provide it to this function.
There was a problem hiding this comment.
I think I originally implemented it like that. Will reconsider though.
EDIT: Here's more context:
I originally thought that we just needed the OS image tag name (e.g., rhel-coreos) instead of the OS image pullspec. To resolve that from the OSImageStream, I also needed the ImageStream itself so I could look it up. As it turns out, the image tag name is unimportant. So with that in mind, I changed it to something similar to what you suggested.
|
@pablintino Putting this here for posterity: On its own, this code doesn't adequately handle OKD. I noticed that #5714 does handle it much better. So I created a separate branch and cherry-picked it locally. Once I did that, this code handled OKD with no issues. |
cheesesashimi
force-pushed
the
zzlotnik/add-osimagestream-generation-cmd
branch
5 times, most recently
from
6c494b6 to
75493d7
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
cheesesashimi
force-pushed
the
zzlotnik/add-osimagestream-generation-cmd
branch
from
75493d7 to
ccbbd52
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
cmd/machine-config-osimagestream/get.go (1)
145-151:klog.Exitfbypasses Cobra's error handling.Using
klog.Exitfterminates the process immediately, bypassing Cobra's error handling and theRunEreturn path. This prevents cleanup code or consistent error formatting. Consider returning an error instead.♻️ Suggested fix
func validateCLIArgs(_ *cobra.Command, _ []string) error { flag.Set("logtostderr", "true") flag.Parse() // To help debugging, show version klog.V(defaultLogVerbosity).Infof("Version: %+v (%s)", version.Raw, version.Hash) // Validate mutually exclusive flags if o.imageStreamPath != "" && o.releaseImage != "" { - klog.Exitf("--imagestream and --release-image are mutually exclusive") + return fmt.Errorf("--imagestream and --release-image are mutually exclusive") } if o.imageStreamPath == "" && o.releaseImage == "" { - klog.Exitf("either --imagestream or --release-image must be provided") + return fmt.Errorf("either --imagestream or --release-image must be provided") } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/machine-config-osimagestream/get.go` around lines 145 - 151, The current checks use klog.Exitf which force-exits the process and bypasses Cobra's RunE/error handling; change the two klog.Exitf calls in the block that inspects o.imageStreamPath and o.releaseImage to return descriptive errors (e.g., fmt.Errorf("...") or errors.New("...")) from the enclosing function (the command's RunE or validation function) so Cobra can handle formatting/cleanup consistently; ensure you update the caller to propagate/return that error rather than exiting the process.cmd/machine-config-osimagestream/version.go (1)
28-28: Variable shadows imported package name.The local variable
versionshadows the importedgithub.com/openshift/machine-config-operator/pkg/versionpackage. While this compiles correctly, it reduces clarity.♻️ Suggested rename
- version := version.Raw + "-" + version.Hash + versionStr := version.Raw + "-" + version.Hash - fmt.Println(componentName, version) + fmt.Println(componentName, versionStr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/machine-config-osimagestream/version.go` at line 28, The local variable named `version` shadows the imported package `version`; rename the local to something like `versionStr` or `fullVersion` and update any subsequent uses accordingly. Specifically, change the assignment that uses `version.Raw` and `version.Hash` (currently `version := version.Raw + "-" + version.Hash`) to assign to the new identifier (e.g., `versionStr`) and replace references to the old local `version` elsewhere in this file so they now reference the new name.cmd/machine-config-osimagestream/helpers.go (1)
96-98: Empty name check may be overly restrictive.The wrapper rejects empty
namewith an error, but perpkg/osimagestream/helpers.go:22-24,GetOSImageStreamSetByNametreats an empty name as a request for the default stream. This wrapper changes that behavior. If the intent is to always require an explicit name here, consider documenting this difference; otherwise, remove this check to preserve the original semantic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/machine-config-osimagestream/helpers.go` around lines 96 - 98, The wrapper currently returns an error when name == "" which overrides the intended behavior of GetOSImageStreamSetByName that treats an empty name as a request for the default stream; remove the empty-name guard in the wrapper so it forwards an empty name through to GetOSImageStreamSetByName (or, if you truly intend to require an explicit name, add a clear comment on the wrapper indicating the deliberate deviation and update callers accordingly). Ensure the change touches the wrapper function that contains the `if name == "" { return nil, fmt.Errorf("empty OSImageStream name") }` check and leaves GetOSImageStreamSetByName unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/machine-config-osimagestream/README.md`:
- Around line 73-75: README currently documents NO_PROXY as supported but
helpers.go (around the NO_PROXY handling) explicitly does not implement it;
update the README entry for NO_PROXY to reflect that NO_PROXY is not supported
(or remove the NO_PROXY bullet entirely) and add a short note pointing to the
limitation in helpers.go (the NO_PROXY handling) so users aren’t misled about
support.
In `@pkg/imageutils/sys_context_fs.go`:
- Around line 79-101: The loader in SysContextPaths.loadPerHostCertificates
currently walks the entire PerHostCertDir tree, reads every file and collapses
names to filepath.Base(path), losing host-relative layout and including non-CA
files; change it to only accept CA files (e.g. .crt/.pem extensions or other
supported CA filenames) and preserve the host-relative path when creating the
ImageRegistryBundle entries (use filepath.Rel to compute the path under
PerHostCertDir and store that relative path in the ImageRegistryBundle.File
instead of filepath.Base), and skip directories and non-matching files so
buildCerts() and ca-bundle.crt generation only consume intended CA bundles.
- Around line 36-51: The current block in SysContextBuilder
(pkg/imageutils/sys_context_fs.go) only loads s.AdditionalTrustBundles when
s.PerHostCertDir != "", causing AdditionalTrustBundles to be ignored if
PerHostCertDir is empty; split the logic so you always load and set
cfg.Spec.AdditionalTrustBundle when len(s.AdditionalTrustBundles) > 0 (use
newTrustBundleLoader().loadAll(...) and assign to
cfg.Spec.AdditionalTrustBundle), and separately load per-host certs (call
s.loadPerHostCertificates() and set cfg.Spec.ImageRegistryBundleData) only when
s.PerHostCertDir != ""; ensure both are applied when both inputs are present.
---
Nitpick comments:
In `@cmd/machine-config-osimagestream/get.go`:
- Around line 145-151: The current checks use klog.Exitf which force-exits the
process and bypasses Cobra's RunE/error handling; change the two klog.Exitf
calls in the block that inspects o.imageStreamPath and o.releaseImage to return
descriptive errors (e.g., fmt.Errorf("...") or errors.New("...")) from the
enclosing function (the command's RunE or validation function) so Cobra can
handle formatting/cleanup consistently; ensure you update the caller to
propagate/return that error rather than exiting the process.
In `@cmd/machine-config-osimagestream/helpers.go`:
- Around line 96-98: The wrapper currently returns an error when name == ""
which overrides the intended behavior of GetOSImageStreamSetByName that treats
an empty name as a request for the default stream; remove the empty-name guard
in the wrapper so it forwards an empty name through to GetOSImageStreamSetByName
(or, if you truly intend to require an explicit name, add a clear comment on the
wrapper indicating the deliberate deviation and update callers accordingly).
Ensure the change touches the wrapper function that contains the `if name == ""
{ return nil, fmt.Errorf("empty OSImageStream name") }` check and leaves
GetOSImageStreamSetByName unchanged.
In `@cmd/machine-config-osimagestream/version.go`:
- Line 28: The local variable named `version` shadows the imported package
`version`; rename the local to something like `versionStr` or `fullVersion` and
update any subsequent uses accordingly. Specifically, change the assignment that
uses `version.Raw` and `version.Hash` (currently `version := version.Raw + "-" +
version.Hash`) to assign to the new identifier (e.g., `versionStr`) and replace
references to the old local `version` elsewhere in this file so they now
reference the new name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca6dbae9-8d45-4c4f-a40d-7948d0fa69b8
📒 Files selected for processing (10)
Makefilecmd/machine-config-osimagestream/README.mdcmd/machine-config-osimagestream/get.gocmd/machine-config-osimagestream/helpers.gocmd/machine-config-osimagestream/main.gocmd/machine-config-osimagestream/version.gopkg/imageutils/sys_context_fs.gopkg/imageutils/sys_context_fs_test.gopkg/imageutils/sys_context_test.gopkg/secrets/secrets_test.go
cheesesashimi
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@cheesesashimi: This pull request references MCO-2163 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Assisted-By: Claude Sonnet 4.5
Ensures that we get an error whenever a K8s secret in either JSON or YAML format is missing either the API version or Kind field.
cheesesashimi
force-pushed
the
zzlotnik/add-osimagestream-generation-cmd
branch
from
ccbbd52 to
c9cb2a1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/machine-config-osimagestream/helpers.go (2)
119-125: Proxy environment variables are case-sensitive; consider checking lowercase variants.Standard conventions support both
HTTP_PROXY/HTTPS_PROXYand their lowercase equivalents (http_proxy/https_proxy). Many containerized environments use lowercase. Missing these could cause proxy configurations to be silently ignored.♻️ Proposed fix
func getProxyConfig() *configv1.ProxyStatus { proxyStatus := &configv1.ProxyStatus{} - if httpProxy := os.Getenv("HTTP_PROXY"); httpProxy != "" { + httpProxy := os.Getenv("HTTP_PROXY") + if httpProxy == "" { + httpProxy = os.Getenv("http_proxy") + } + if httpProxy != "" { proxyStatus.HTTPProxy = httpProxy } - if httpsProxy := os.Getenv("HTTPS_PROXY"); httpsProxy != "" { + httpsProxy := os.Getenv("HTTPS_PROXY") + if httpsProxy == "" { + httpsProxy = os.Getenv("https_proxy") + } + if httpsProxy != "" { proxyStatus.HTTPSProxy = httpsProxy }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/machine-config-osimagestream/helpers.go` around lines 119 - 125, The code only reads uppercase proxy env vars (HTTP_PROXY/HTTPS_PROXY) so lowercase variants (http_proxy/https_proxy) used in many environments are ignored; update the logic that sets proxyStatus.HTTPProxy and proxyStatus.HTTPSProxy to check lowercase fallbacks when the uppercase vars are empty (e.g., read os.Getenv("HTTP_PROXY") || os.Getenv("http_proxy") and similarly for HTTPS), ensuring proxyStatus is populated from either variant while keeping existing behavior that prefers explicit uppercase values.
36-38: Consider moving output format validation closer to its usage.The
outputFormatvalidation here is only relevant forrunOSImageStreamCmd(which callswriteOutput), but this function is also called byrunDefaultNodeImageCmdandrunOSImagePullspecCmdwhich don't useoutputFormat. While benign since the default is valid, this couples unrelated concerns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/machine-config-osimagestream/helpers.go` around lines 36 - 38, The outputFormat check currently in the helper (the code that inspects opts.outputFormat) should be removed from that shared helper and relocated closer to where it's used: either validate inside writeOutput or in runOSImageStreamCmd before calling writeOutput; keep runDefaultNodeImageCmd and runOSImagePullspecCmd unaffected. Specifically, delete the unsupported format error from the shared helper, and add a validation that accepts only outputFormatJSON or outputFormatYAML inside writeOutput (or immediately in runOSImageStreamCmd) using the same error message pattern referencing opts.outputFormat so callers that don't use outputFormat are no longer coupled to this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/machine-config-osimagestream/helpers.go`:
- Around line 119-125: The code only reads uppercase proxy env vars
(HTTP_PROXY/HTTPS_PROXY) so lowercase variants (http_proxy/https_proxy) used in
many environments are ignored; update the logic that sets proxyStatus.HTTPProxy
and proxyStatus.HTTPSProxy to check lowercase fallbacks when the uppercase vars
are empty (e.g., read os.Getenv("HTTP_PROXY") || os.Getenv("http_proxy") and
similarly for HTTPS), ensuring proxyStatus is populated from either variant
while keeping existing behavior that prefers explicit uppercase values.
- Around line 36-38: The outputFormat check currently in the helper (the code
that inspects opts.outputFormat) should be removed from that shared helper and
relocated closer to where it's used: either validate inside writeOutput or in
runOSImageStreamCmd before calling writeOutput; keep runDefaultNodeImageCmd and
runOSImagePullspecCmd unaffected. Specifically, delete the unsupported format
error from the shared helper, and add a validation that accepts only
outputFormatJSON or outputFormatYAML inside writeOutput (or immediately in
runOSImageStreamCmd) using the same error message pattern referencing
opts.outputFormat so callers that don't use outputFormat are no longer coupled
to this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2501d7c4-8b4a-4bdd-a9fa-fbbdcd2a425f
📒 Files selected for processing (10)
Makefilecmd/machine-config-osimagestream/README.mdcmd/machine-config-osimagestream/get.gocmd/machine-config-osimagestream/helpers.gocmd/machine-config-osimagestream/main.gocmd/machine-config-osimagestream/version.gopkg/imageutils/sys_context_fs.gopkg/imageutils/sys_context_fs_test.gopkg/imageutils/sys_context_test.gopkg/secrets/secrets_test.go
✅ Files skipped from review due to trivial changes (3)
- pkg/imageutils/sys_context_test.go
- cmd/machine-config-osimagestream/README.md
- cmd/machine-config-osimagestream/main.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/secrets/secrets_test.go
- Makefile
- cmd/machine-config-osimagestream/version.go
- pkg/imageutils/sys_context_fs_test.go
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi, pablintino The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cheesesashimi
changed the title
|
@cheesesashimi: This pull request references MCO-2163 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Verification
In order to check that the binary works fine in environments with proxy and disconnected environments we copied the binary to a master node in a disconnected cluster and we checked that the binary was working fine and was able to handle the proxy configuration and the registries.conf file and the certs To check the proxy and the registries.conf we used /etc/mco/proxy.env and /etc/containers/registries.conf in a disconnected cluster with proxy and imagecontentsourcepolicies. To check the certs we used --cert-dir And the /verified by @sergiordlr |
openshift-ci-robot
added
the
verified
|
@sergiordlr: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@cheesesashimi: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
- What I did
This introduces a new helper binary called
machine-config-osimagestreamthat will examine either the given release image pullspec or the provided ImageStream file to create an OSImageStream object in either JSON or YAML format. This is mostly intended to be used by the installer bootstrap process, but could also be used to help debug the OSImageStream produced by a given release image or ImageStream file.There is also a
default-node-imagemode which will get the OSImageStream and then return the image pullspec for the default OS image. Similarly, there isos-image-pullspecwhich will do the same thing but it will look up the given OSImageStream name and return the OS image pullspec that goes with it.- How to verify it
go build -o machine-config-osimagestream ./cmd/machine-config-osimagestream./machine-config-osimagestream get osimagestream --release-image registry.ci.openshift.org/ocp/release-5:5.0.0-0.nightly-2026-03-16-164911 --pull-secret /path/to/pull/secret./machine-config-osimagestream get default-node-image --release-image registry.ci.openshift.org/ocp/release-5:5.0.0-0.nightly-2026-03-16-164911 --pull-secret /path/to/pull/secret./machine-config-osimagestream get os-image-pullspec --release-image registry.ci.openshift.org/ocp/release-5:5.0.0-0.nightly-2026-03-16-164911 --pull-secret /path/to/pull/secret --osimagestream-name rhel-9- Description for the changelog
Adds machine-config-osimagestream