From cb28d277856bfc6452274b6367b075f8eb1dde54 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Thu, 2 Oct 2025 15:13:31 -0700 Subject: [PATCH 1/2] pkg/cli/admin/upgrade/recommend: Drop obsolete precheckEnabled knob Since 8bdac163b5 (pkg/cli/admin/upgrade/recommend: Enable precheck and accept gates, 2025-09-03, #2088), this knob has always been enabled. Save ourselves a no longer necessary level of indentation by removing it completely. --- .../admin/upgrade/recommend/examples_test.go | 1 - pkg/cli/admin/upgrade/recommend/recommend.go | 73 +++++++++---------- 2 files changed, 34 insertions(+), 40 deletions(-) diff --git a/pkg/cli/admin/upgrade/recommend/examples_test.go b/pkg/cli/admin/upgrade/recommend/examples_test.go index 96f9ff7ce2..0129d31aee 100644 --- a/pkg/cli/admin/upgrade/recommend/examples_test.go +++ b/pkg/cli/admin/upgrade/recommend/examples_test.go @@ -91,7 +91,6 @@ func TestExamples(t *testing.T) { if err := opts.Complete(nil, nil, nil); err != nil { t.Fatalf("Error when completing options: %v", err) } - opts.precheckEnabled = true var stdout, stderr bytes.Buffer opts.Out = &stdout diff --git a/pkg/cli/admin/upgrade/recommend/recommend.go b/pkg/cli/admin/upgrade/recommend/recommend.go index 0a39bc1994..89c54ea00f 100644 --- a/pkg/cli/admin/upgrade/recommend/recommend.go +++ b/pkg/cli/admin/upgrade/recommend/recommend.go @@ -76,7 +76,6 @@ type options struct { mockData mockData showOutdatedReleases bool - precheckEnabled bool // quiet configures the verbosity of output. When 'quiet' is true and 'version' is set, only print unaccepted issue names. quiet bool @@ -134,8 +133,6 @@ func (o *options) Complete(f kcmdutil.Factory, cmd *cobra.Command, args []string } } - o.precheckEnabled = true - return nil } @@ -188,47 +185,45 @@ func (o *options) Run(ctx context.Context) error { issues.Insert(string(configv1.OperatorProgressing)) } - if o.precheckEnabled { - conditions, err := o.precheck(ctx) - if err != nil { - if !o.quiet { - fmt.Fprintf(o.Out, "Failed to check for at least some preconditions: %v\n", err) - } - issues.Insert("FailedToCompletePrecheck") - } - var happyConditions []string - var acceptedConditions []string - var unhappyConditions []string - for _, condition := range conditions { - if condition.Status == metav1.ConditionTrue { - happyConditions = append(happyConditions, fmt.Sprintf("%s (%s)", condition.Type, condition.Reason)) + conditions, err := o.precheck(ctx) + if err != nil { + if !o.quiet { + fmt.Fprintf(o.Out, "Failed to check for at least some preconditions: %v\n", err) + } + issues.Insert("FailedToCompletePrecheck") + } + var happyConditions []string + var acceptedConditions []string + var unhappyConditions []string + for _, condition := range conditions { + if condition.Status == metav1.ConditionTrue { + happyConditions = append(happyConditions, fmt.Sprintf("%s (%s)", condition.Type, condition.Reason)) + } else { + issues.Insert(condition.acceptanceName) + if accept.Has(condition.acceptanceName) { + acceptedConditions = append(acceptedConditions, condition.Type) } else { - issues.Insert(condition.acceptanceName) - if accept.Has(condition.acceptanceName) { - acceptedConditions = append(acceptedConditions, condition.Type) - } else { - unhappyConditions = append(unhappyConditions, condition.Type) - } + unhappyConditions = append(unhappyConditions, condition.Type) } } + } - if !o.quiet { - if len(happyConditions) > 0 { - sort.Strings(happyConditions) - fmt.Fprintf(o.Out, "The following conditions found no cause for concern in updating this cluster to later releases: %s\n\n", strings.Join(happyConditions, ", ")) - } - if len(acceptedConditions) > 0 { - sort.Strings(acceptedConditions) - fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases, but were explicitly accepted via --accept: %s\n\n", strings.Join(acceptedConditions, ", ")) - } - if len(unhappyConditions) > 0 { - sort.Strings(unhappyConditions) - fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases: %s\n\n", strings.Join(unhappyConditions, ", ")) + if !o.quiet { + if len(happyConditions) > 0 { + sort.Strings(happyConditions) + fmt.Fprintf(o.Out, "The following conditions found no cause for concern in updating this cluster to later releases: %s\n\n", strings.Join(happyConditions, ", ")) + } + if len(acceptedConditions) > 0 { + sort.Strings(acceptedConditions) + fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases, but were explicitly accepted via --accept: %s\n\n", strings.Join(acceptedConditions, ", ")) + } + if len(unhappyConditions) > 0 { + sort.Strings(unhappyConditions) + fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases: %s\n\n", strings.Join(unhappyConditions, ", ")) - for _, c := range conditions { - if c.Status != metav1.ConditionTrue { - fmt.Fprintf(o.Out, "%s=%s:\n\n Reason: %s\n Message: %s\n\n", c.Type, c.Status, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) - } + for _, c := range conditions { + if c.Status != metav1.ConditionTrue { + fmt.Fprintf(o.Out, "%s=%s:\n\n Reason: %s\n Message: %s\n\n", c.Type, c.Status, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) } } } From ecf80c8dc545ab1144d8cc553b277a9cfac6f975 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Wed, 18 Jun 2025 15:00:55 -0700 Subject: [PATCH 2/2] pkg/cli/admin/upgrade/recommend: Add a blank line after --version conditional risk When I added the: Update to 4.18.6 Recommended=False: ... block in c1ef32845c (pkg/cli/admin/upgrade/recommend: Add --version option for specific releases, 2024-10-14, #1897), there was no subsequent output in this "--version has conditional risks" case, so the earlier lack-of-blank-line made sense. But then 2d7e00420c (pkg/cli/admin/upgrade/recommend: New, feature-gated --accept, 2025-05-06, #2023) gave the option for a subsequent: error: issues that apply to this cluster but which were not included in --accept: ... or: Update to %s has no known issues relevant... This commit adds trackers to all the output blocks in the command for "was there previous output on this stream?", so that a new block (or a returned error) can be prefixed by the blank line needed to set off the new block from the previous block. For example, it now sets off the error line from the previous, possibly-long message strings: ... Update to 4.18.6 Recommended=False: Image: quay.io/openshift-release-dev/ocp-release@sha256:61fdad894f035a8b192647c224faf565279518255bdbf60a91db4ee0479adaa6 Release URL: https://access.redhat.com/errata/RHSA-2025:3066 Reason: CRIOLayerCompressionPulls Message: The CRI-O container runtime may fail to pull images with certain layer compression characteristics https://issues.redhat.com/browse/OCPNODE-3074 error: issues that apply to this cluster but which were not included in --accept: ConditionalUpdateRisk,KubeContainerWaiting instead of cramming that 'error: ...' or '... has no known issues...' right onto the end of the output. The error is getting written to standard error instead of the standard output, so there's still some wiggle room for poor spacing for terminal users and others who are flattening those two streams together. And now that we have the additional newline handling in recommend.go, I can drop the newline I'd been manually injecting in examples_test.go to make the test fixtures more readable. I should have noticed that test-specific injection as a sign that the better recommend.go newline handling was useful. Oh well, better late than never. --- .../admin/upgrade/recommend/examples_test.go | 2 +- pkg/cli/admin/upgrade/recommend/recommend.go | 115 +++++++++++++++--- 2 files changed, 102 insertions(+), 15 deletions(-) diff --git a/pkg/cli/admin/upgrade/recommend/examples_test.go b/pkg/cli/admin/upgrade/recommend/examples_test.go index 0129d31aee..dacf1d8d2a 100644 --- a/pkg/cli/admin/upgrade/recommend/examples_test.go +++ b/pkg/cli/admin/upgrade/recommend/examples_test.go @@ -97,7 +97,7 @@ func TestExamples(t *testing.T) { opts.ErrOut = &stderr if err := opts.Run(context.Background()); err != nil { - compareWithFixture(t, bytes.Join([][]byte{stdout.Bytes(), []byte("\nerror: "), []byte(err.Error()), []byte("\n")}, []byte{}), cv, variant.outputSuffix) + compareWithFixture(t, bytes.Join([][]byte{stdout.Bytes(), []byte("error: "), []byte(err.Error()), []byte("\n")}, []byte{}), cv, variant.outputSuffix) } else { compareWithFixture(t, stdout.Bytes(), cv, variant.outputSuffix) } diff --git a/pkg/cli/admin/upgrade/recommend/recommend.go b/pkg/cli/admin/upgrade/recommend/recommend.go index 89c54ea00f..d6f0460d92 100644 --- a/pkg/cli/admin/upgrade/recommend/recommend.go +++ b/pkg/cli/admin/upgrade/recommend/recommend.go @@ -139,6 +139,7 @@ func (o *options) Complete(f kcmdutil.Factory, cmd *cobra.Command, args []string func (o *options) Run(ctx context.Context) error { issues := sets.New[string]() accept := sets.New[string](o.accept...) + var previousStdout, previousStderr bool var cv *configv1.ClusterVersion if cv = o.mockData.clusterVersion; cv == nil { @@ -159,7 +160,8 @@ func (o *options) Run(ctx context.Context) error { acceptContext = "accepted " } if !o.quiet { - fmt.Fprintf(o.Out, "%s%s=%s:\n\n Reason: %s\n Message: %s\n\n", acceptContext, c.Type, c.Status, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) + fmt.Fprintf(o.Out, "%s%s=%s:\n\n Reason: %s\n Message: %s\n", acceptContext, c.Type, c.Status, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) + previousStdout = true } issues.Insert(string(clusterStatusFailing)) } @@ -170,6 +172,7 @@ func (o *options) Run(ctx context.Context) error { } if !o.quiet { fmt.Fprintf(o.ErrOut, "%swarning: No current %s info, see `oc describe clusterversion` for more details.\n", acceptContext, clusterStatusFailing) + previousStderr = true } issues.Insert(string(clusterStatusFailing)) } @@ -180,7 +183,11 @@ func (o *options) Run(ctx context.Context) error { acceptContext = "accepted " } if !o.quiet { - fmt.Fprintf(o.Out, "%sinfo: An update is in progress. You may wish to let this update complete before requesting a new update.\n %s\n\n", acceptContext, strings.ReplaceAll(c.Message, "\n", "\n ")) + if previousStdout { + fmt.Fprintln(o.Out) + } + fmt.Fprintf(o.Out, "%sinfo: An update is in progress. You may wish to let this update complete before requesting a new update.\n %s\n", acceptContext, strings.ReplaceAll(c.Message, "\n", "\n ")) + previousStdout = true } issues.Insert(string(configv1.OperatorProgressing)) } @@ -188,7 +195,11 @@ func (o *options) Run(ctx context.Context) error { conditions, err := o.precheck(ctx) if err != nil { if !o.quiet { + if previousStdout { + fmt.Fprintln(o.Out) + } fmt.Fprintf(o.Out, "Failed to check for at least some preconditions: %v\n", err) + previousStdout = true } issues.Insert("FailedToCompletePrecheck") } @@ -211,19 +222,35 @@ func (o *options) Run(ctx context.Context) error { if !o.quiet { if len(happyConditions) > 0 { sort.Strings(happyConditions) - fmt.Fprintf(o.Out, "The following conditions found no cause for concern in updating this cluster to later releases: %s\n\n", strings.Join(happyConditions, ", ")) + if previousStdout { + fmt.Fprintln(o.Out) + } + fmt.Fprintf(o.Out, "The following conditions found no cause for concern in updating this cluster to later releases: %s\n", strings.Join(happyConditions, ", ")) + previousStdout = true } if len(acceptedConditions) > 0 { sort.Strings(acceptedConditions) - fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases, but were explicitly accepted via --accept: %s\n\n", strings.Join(acceptedConditions, ", ")) + if previousStdout { + fmt.Fprintln(o.Out) + } + fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases, but were explicitly accepted via --accept: %s\n", strings.Join(acceptedConditions, ", ")) + previousStdout = true } if len(unhappyConditions) > 0 { sort.Strings(unhappyConditions) - fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases: %s\n\n", strings.Join(unhappyConditions, ", ")) + if previousStdout { + fmt.Fprintln(o.Out) + } + fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases: %s\n", strings.Join(unhappyConditions, ", ")) + previousStdout = true for _, c := range conditions { if c.Status != metav1.ConditionTrue { - fmt.Fprintf(o.Out, "%s=%s:\n\n Reason: %s\n Message: %s\n\n", c.Type, c.Status, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) + if previousStdout { + fmt.Fprintln(o.Out) + } + fmt.Fprintf(o.Out, "%s=%s:\n\n Reason: %s\n Message: %s\n", c.Type, c.Status, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) + previousStdout = true } } } @@ -231,13 +258,20 @@ func (o *options) Run(ctx context.Context) error { if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.RetrievedUpdates); c != nil && c.Status != configv1.ConditionTrue { if !o.quiet { - fmt.Fprintf(o.ErrOut, "warning: Cannot refresh available updates:\n Reason: %s\n Message: %s\n\n", c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) + if previousStderr { + fmt.Fprintln(o.ErrOut) + } + fmt.Fprintf(o.ErrOut, "warning: Cannot refresh available updates:\n Reason: %s\n Message: %s\n", c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) + previousStderr = true } issues.Insert("CannotRetrieveUpdates") } if !o.quiet { if cv.Spec.Channel != "" { + if previousStdout { + fmt.Fprintln(o.Out) + } if cv.Spec.Upstream == "" { fmt.Fprint(o.Out, "Upstream update service is unset, so the cluster will use an appropriate default.\n") } else { @@ -248,6 +282,7 @@ func (o *options) Run(ctx context.Context) error { } else { fmt.Fprintf(o.Out, "Channel: %s\n", cv.Spec.Channel) } + previousStdout = true } } @@ -257,7 +292,11 @@ func (o *options) Run(ctx context.Context) error { version, err := semver.Parse(update.Release.Version) if err != nil { if !o.quiet { - fmt.Fprintf(o.ErrOut, "warning: Cannot parse SemVer available update %q: %v", update.Release.Version, err) + if previousStderr { + fmt.Fprintln(o.ErrOut) + } + fmt.Fprintf(o.ErrOut, "warning: Cannot parse SemVer available update %q: %v\n", update.Release.Version, err) + previousStderr = true } continue } @@ -284,7 +323,11 @@ func (o *options) Run(ctx context.Context) error { version, err := semver.Parse(update.Version) if err != nil { if !o.quiet { - fmt.Fprintf(o.ErrOut, "warning: Cannot parse SemVer available update %q: %v", update.Version, err) + if previousStderr { + fmt.Fprintln(o.ErrOut) + } + fmt.Fprintf(o.ErrOut, "warning: Cannot parse SemVer available update %q: %v\n", update.Version, err) + previousStderr = true } continue } @@ -301,29 +344,49 @@ func (o *options) Run(ctx context.Context) error { if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorUpgradeable); c != nil && c.Status == configv1.ConditionFalse { if err := injectUpgradeableAsCondition(cv.Status.Desired.Version, c, majorMinorBuckets); err != nil && !o.quiet { if !o.quiet { - fmt.Fprintf(o.ErrOut, "warning: Cannot inject %s=%s as a conditional update risk: %s\n\nReason: %s\n Message: %s\n\n", c.Type, c.Status, err, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) + if previousStderr { + fmt.Fprintln(o.ErrOut) + } + fmt.Fprintf(o.ErrOut, "warning: Cannot inject %s=%s as a conditional update risk: %s\n\nReason: %s\n Message: %s\n", c.Type, c.Status, err, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) + previousStderr = true } } } if o.version != nil { if len(majorMinorBuckets) == 0 { + if previousStdout { + fmt.Fprintln(o.Out) + } else if previousStderr { + fmt.Fprintln(o.ErrOut) + } return fmt.Errorf("no updates available, so cannot display context for the requested release %s", o.version) } if major, ok := majorMinorBuckets[o.version.Major]; !ok { + if previousStdout { + fmt.Fprintln(o.Out) + } else if previousStderr { + fmt.Fprintln(o.ErrOut) + } return fmt.Errorf("no updates to %d available, so cannot display context for the requested release %s", o.version.Major, o.version) } else if minor, ok := major[o.version.Minor]; !ok { + if previousStdout { + fmt.Fprintln(o.Out) + } else if previousStderr { + fmt.Fprintln(o.ErrOut) + } return fmt.Errorf("no updates to %d.%d available, so cannot display context for the requested release %s", o.version.Major, o.version.Minor, o.version) } else { for _, update := range minor { if update.Release.Version == o.version.String() { - if !o.quiet { - fmt.Fprintln(o.Out) - } if c := notRecommendedCondition(update); c == nil { if !o.quiet { + if previousStdout { + fmt.Fprintln(o.Out) + } fmt.Fprintf(o.Out, "Update to %s has no known issues relevant to this cluster.\nImage: %s\nRelease URL: %s\n", update.Release.Version, update.Release.Image, update.Release.URL) + previousStdout = true } } else { if !o.quiet { @@ -331,24 +394,45 @@ func (o *options) Run(ctx context.Context) error { if accept.Has("ConditionalUpdateRisk") { reason = fmt.Sprintf("accepted %s via ConditionalUpdateRisk", c.Reason) } + if previousStdout { + fmt.Fprintln(o.Out) + } fmt.Fprintf(o.Out, "Update to %s %s=%s:\nImage: %s\nRelease URL: %s\nReason: %s\nMessage: %s\n", update.Release.Version, c.Type, c.Status, update.Release.Image, update.Release.URL, reason, strings.ReplaceAll(c.Message, "\n", "\n ")) + previousStdout = true } issues.Insert("ConditionalUpdateRisk") } unaccepted := issues.Difference(accept) if unaccepted.Len() > 0 { + if previousStdout { + fmt.Fprintln(o.Out) + } else if previousStderr { + fmt.Fprintln(o.ErrOut) + } return fmt.Errorf("issues that apply to this cluster but which were not included in --accept: %s", strings.Join(sets.List(unaccepted), ",")) } else if issues.Len() > 0 && !o.quiet { + if previousStdout { + fmt.Fprintln(o.Out) + } fmt.Fprintf(o.Out, "Update to %s has no known issues relevant to this cluster other than the accepted %s.\n", update.Release.Version, strings.Join(sets.List(issues), ",")) + previousStdout = true } return nil } } + if previousStdout { + fmt.Fprintln(o.Out) + } else if previousStderr { + fmt.Fprintln(o.ErrOut) + } return fmt.Errorf("no update to %s available, so cannot display context for the requested release", o.version) } } if len(majorMinorBuckets) == 0 { + if previousStdout { + fmt.Fprintln(o.Out) + } fmt.Fprintf(o.Out, "No updates available. You may still upgrade to a specific release image with --to-image or wait for new updates to be available.\n") return nil } @@ -369,8 +453,11 @@ func (o *options) Run(ctx context.Context) error { return minors[i] > minors[j] // sort descending, minor updates bring both feature and bugfixes }) for _, minor := range minors { - fmt.Fprintln(o.Out) + if previousStdout { + fmt.Fprintln(o.Out) + } fmt.Fprintf(o.Out, "Updates to %d.%d:\n", major, minor) + previousStdout = true lastWasLong := false headerQueued := true