Skip to content

Commit ecf80c8

Browse files
wkingwking
committed
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 c1ef328 (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 2d7e004 (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.
1 parent cb28d27 commit ecf80c8

File tree

2 files changed

+102
-15
lines changed

2 files changed

+102
-15
lines changed

‎pkg/cli/admin/upgrade/recommend/examples_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ func TestExamples(t *testing.T) {
9797
opts.ErrOut = &stderr
9898

9999
if err := opts.Run(context.Background()); err != nil {
100-
compareWithFixture(t, bytes.Join([][]byte{stdout.Bytes(), []byte("\nerror: "), []byte(err.Error()), []byte("\n")}, []byte{}), cv, variant.outputSuffix)
100+
compareWithFixture(t, bytes.Join([][]byte{stdout.Bytes(), []byte("error: "), []byte(err.Error()), []byte("\n")}, []byte{}), cv, variant.outputSuffix)
101101
} else {
102102
compareWithFixture(t, stdout.Bytes(), cv, variant.outputSuffix)
103103
}

‎pkg/cli/admin/upgrade/recommend/recommend.go‎

Lines changed: 101 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ func (o *options) Complete(f kcmdutil.Factory, cmd *cobra.Command, args []string
139139
func (o *options) Run(ctx context.Context) error {
140140
issues := sets.New[string]()
141141
accept := sets.New[string](o.accept...)
142+
var previousStdout, previousStderr bool
142143

143144
var cv *configv1.ClusterVersion
144145
if cv = o.mockData.clusterVersion; cv == nil {
@@ -159,7 +160,8 @@ func (o *options) Run(ctx context.Context) error {
159160
acceptContext = "accepted "
160161
}
161162
if !o.quiet {
162-
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 "))
163+
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 "))
164+
previousStdout = true
163165
}
164166
issues.Insert(string(clusterStatusFailing))
165167
}
@@ -170,6 +172,7 @@ func (o *options) Run(ctx context.Context) error {
170172
}
171173
if !o.quiet {
172174
fmt.Fprintf(o.ErrOut, "%swarning: No current %s info, see `oc describe clusterversion` for more details.\n", acceptContext, clusterStatusFailing)
175+
previousStderr = true
173176
}
174177
issues.Insert(string(clusterStatusFailing))
175178
}
@@ -180,15 +183,23 @@ func (o *options) Run(ctx context.Context) error {
180183
acceptContext = "accepted "
181184
}
182185
if !o.quiet {
183-
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 "))
186+
if previousStdout {
187+
fmt.Fprintln(o.Out)
188+
}
189+
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 "))
190+
previousStdout = true
184191
}
185192
issues.Insert(string(configv1.OperatorProgressing))
186193
}
187194

188195
conditions, err := o.precheck(ctx)
189196
if err != nil {
190197
if !o.quiet {
198+
if previousStdout {
199+
fmt.Fprintln(o.Out)
200+
}
191201
fmt.Fprintf(o.Out, "Failed to check for at least some preconditions: %v\n", err)
202+
previousStdout = true
192203
}
193204
issues.Insert("FailedToCompletePrecheck")
194205
}
@@ -211,33 +222,56 @@ func (o *options) Run(ctx context.Context) error {
211222
if !o.quiet {
212223
if len(happyConditions) > 0 {
213224
sort.Strings(happyConditions)
214-
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, ", "))
225+
if previousStdout {
226+
fmt.Fprintln(o.Out)
227+
}
228+
fmt.Fprintf(o.Out, "The following conditions found no cause for concern in updating this cluster to later releases: %s\n", strings.Join(happyConditions, ", "))
229+
previousStdout = true
215230
}
216231
if len(acceptedConditions) > 0 {
217232
sort.Strings(acceptedConditions)
218-
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, ", "))
233+
if previousStdout {
234+
fmt.Fprintln(o.Out)
235+
}
236+
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, ", "))
237+
previousStdout = true
219238
}
220239
if len(unhappyConditions) > 0 {
221240
sort.Strings(unhappyConditions)
222-
fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases: %s\n\n", strings.Join(unhappyConditions, ", "))
241+
if previousStdout {
242+
fmt.Fprintln(o.Out)
243+
}
244+
fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases: %s\n", strings.Join(unhappyConditions, ", "))
245+
previousStdout = true
223246

224247
for _, c := range conditions {
225248
if c.Status != metav1.ConditionTrue {
226-
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 "))
249+
if previousStdout {
250+
fmt.Fprintln(o.Out)
251+
}
252+
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 "))
253+
previousStdout = true
227254
}
228255
}
229256
}
230257
}
231258

232259
if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.RetrievedUpdates); c != nil && c.Status != configv1.ConditionTrue {
233260
if !o.quiet {
234-
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 "))
261+
if previousStderr {
262+
fmt.Fprintln(o.ErrOut)
263+
}
264+
fmt.Fprintf(o.ErrOut, "warning: Cannot refresh available updates:\n Reason: %s\n Message: %s\n", c.Reason, strings.ReplaceAll(c.Message, "\n", "\n "))
265+
previousStderr = true
235266
}
236267
issues.Insert("CannotRetrieveUpdates")
237268
}
238269

239270
if !o.quiet {
240271
if cv.Spec.Channel != "" {
272+
if previousStdout {
273+
fmt.Fprintln(o.Out)
274+
}
241275
if cv.Spec.Upstream == "" {
242276
fmt.Fprint(o.Out, "Upstream update service is unset, so the cluster will use an appropriate default.\n")
243277
} else {
@@ -248,6 +282,7 @@ func (o *options) Run(ctx context.Context) error {
248282
} else {
249283
fmt.Fprintf(o.Out, "Channel: %s\n", cv.Spec.Channel)
250284
}
285+
previousStdout = true
251286
}
252287
}
253288

@@ -257,7 +292,11 @@ func (o *options) Run(ctx context.Context) error {
257292
version, err := semver.Parse(update.Release.Version)
258293
if err != nil {
259294
if !o.quiet {
260-
fmt.Fprintf(o.ErrOut, "warning: Cannot parse SemVer available update %q: %v", update.Release.Version, err)
295+
if previousStderr {
296+
fmt.Fprintln(o.ErrOut)
297+
}
298+
fmt.Fprintf(o.ErrOut, "warning: Cannot parse SemVer available update %q: %v\n", update.Release.Version, err)
299+
previousStderr = true
261300
}
262301
continue
263302
}
@@ -284,7 +323,11 @@ func (o *options) Run(ctx context.Context) error {
284323
version, err := semver.Parse(update.Version)
285324
if err != nil {
286325
if !o.quiet {
287-
fmt.Fprintf(o.ErrOut, "warning: Cannot parse SemVer available update %q: %v", update.Version, err)
326+
if previousStderr {
327+
fmt.Fprintln(o.ErrOut)
328+
}
329+
fmt.Fprintf(o.ErrOut, "warning: Cannot parse SemVer available update %q: %v\n", update.Version, err)
330+
previousStderr = true
288331
}
289332
continue
290333
}
@@ -301,54 +344,95 @@ func (o *options) Run(ctx context.Context) error {
301344
if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorUpgradeable); c != nil && c.Status == configv1.ConditionFalse {
302345
if err := injectUpgradeableAsCondition(cv.Status.Desired.Version, c, majorMinorBuckets); err != nil && !o.quiet {
303346
if !o.quiet {
304-
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 "))
347+
if previousStderr {
348+
fmt.Fprintln(o.ErrOut)
349+
}
350+
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 "))
351+
previousStderr = true
305352
}
306353
}
307354
}
308355

309356
if o.version != nil {
310357
if len(majorMinorBuckets) == 0 {
358+
if previousStdout {
359+
fmt.Fprintln(o.Out)
360+
} else if previousStderr {
361+
fmt.Fprintln(o.ErrOut)
362+
}
311363
return fmt.Errorf("no updates available, so cannot display context for the requested release %s", o.version)
312364
}
313365

314366
if major, ok := majorMinorBuckets[o.version.Major]; !ok {
367+
if previousStdout {
368+
fmt.Fprintln(o.Out)
369+
} else if previousStderr {
370+
fmt.Fprintln(o.ErrOut)
371+
}
315372
return fmt.Errorf("no updates to %d available, so cannot display context for the requested release %s", o.version.Major, o.version)
316373
} else if minor, ok := major[o.version.Minor]; !ok {
374+
if previousStdout {
375+
fmt.Fprintln(o.Out)
376+
} else if previousStderr {
377+
fmt.Fprintln(o.ErrOut)
378+
}
317379
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)
318380
} else {
319381
for _, update := range minor {
320382
if update.Release.Version == o.version.String() {
321-
if !o.quiet {
322-
fmt.Fprintln(o.Out)
323-
}
324383
if c := notRecommendedCondition(update); c == nil {
325384
if !o.quiet {
385+
if previousStdout {
386+
fmt.Fprintln(o.Out)
387+
}
326388
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)
389+
previousStdout = true
327390
}
328391
} else {
329392
if !o.quiet {
330393
reason := c.Reason
331394
if accept.Has("ConditionalUpdateRisk") {
332395
reason = fmt.Sprintf("accepted %s via ConditionalUpdateRisk", c.Reason)
333396
}
397+
if previousStdout {
398+
fmt.Fprintln(o.Out)
399+
}
334400
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 "))
401+
previousStdout = true
335402
}
336403
issues.Insert("ConditionalUpdateRisk")
337404
}
338405
unaccepted := issues.Difference(accept)
339406
if unaccepted.Len() > 0 {
407+
if previousStdout {
408+
fmt.Fprintln(o.Out)
409+
} else if previousStderr {
410+
fmt.Fprintln(o.ErrOut)
411+
}
340412
return fmt.Errorf("issues that apply to this cluster but which were not included in --accept: %s", strings.Join(sets.List(unaccepted), ","))
341413
} else if issues.Len() > 0 && !o.quiet {
414+
if previousStdout {
415+
fmt.Fprintln(o.Out)
416+
}
342417
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), ","))
418+
previousStdout = true
343419
}
344420
return nil
345421
}
346422
}
423+
if previousStdout {
424+
fmt.Fprintln(o.Out)
425+
} else if previousStderr {
426+
fmt.Fprintln(o.ErrOut)
427+
}
347428
return fmt.Errorf("no update to %s available, so cannot display context for the requested release", o.version)
348429
}
349430
}
350431

351432
if len(majorMinorBuckets) == 0 {
433+
if previousStdout {
434+
fmt.Fprintln(o.Out)
435+
}
352436
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")
353437
return nil
354438
}
@@ -369,8 +453,11 @@ func (o *options) Run(ctx context.Context) error {
369453
return minors[i] > minors[j] // sort descending, minor updates bring both feature and bugfixes
370454
})
371455
for _, minor := range minors {
372-
fmt.Fprintln(o.Out)
456+
if previousStdout {
457+
fmt.Fprintln(o.Out)
458+
}
373459
fmt.Fprintf(o.Out, "Updates to %d.%d:\n", major, minor)
460+
previousStdout = true
374461
lastWasLong := false
375462
headerQueued := true
376463

0 commit comments

Comments
 (0)