Update rad commands for concise output#11773
Update rad commands for concise output#11773zachcasper wants to merge 13 commits intoradius-project:mainfrom
Conversation
Signed-off-by: Zach Casper <zachcasper@microsoft.com>
Signed-off-by: Zach Casper <zachcasper@microsoft.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11773 +/- ##
==========================================
+ Coverage 51.19% 51.23% +0.03%
==========================================
Files 715 714 -1
Lines 45081 45003 -78
==========================================
- Hits 23079 23057 -22
+ Misses 19803 19748 -55
+ Partials 2199 2198 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Zach Casper <zachcasper@microsoft.com>
Signed-off-by: Zach Casper <zachcasper@microsoft.com>
Signed-off-by: Zach Casper <zachcasper@microsoft.com>
DariuszPorowski
left a comment
DariuszPorowski
left a comment
There was a problem hiding this comment.
praise: Great standardization! It provides clean and consistent output.
Radius functional test overviewClick here to see the test run details
Test Status⌛ Building Radius and pushing container images for functional tests... |
| return err | ||
| } | ||
|
|
||
| r.Output.LogInfo("%s/%s created", r.FullyQualifiedResourceTypeName, r.ResourceName) |
There was a problem hiding this comment.
Two issues here:
(a) -o/--output flag is now silently ignored. The flag is still registered (commonflags.AddOutputFlag(cmd) at line 55) and parsed into r.Format (lines 96–100), but the call to WriteFormatted(r.Format, response, ...) was removed and there is no branch on r.Format here. Anyone scripting with rad resource create ... -o json will silently get this concise text line instead of JSON — a behavior regression with no error.
Please either:
- remove
commonflags.AddOutputFlag(cmd), theFormatfield, and theRequireOutputblock; or - branch on
r.Format: emit this line for the default/tableformat, and callWriteFormatted(using the cachedresponsefromCreateOrUpdateResource) forjson/yaml.
(b) New behavior is untested. pkg/cli/cmd/resource/create/create_test.go was not updated in this PR. The only test (Test_Run/Success: resource provider created) checks require.NoError(t, err) but never asserts on outputSink.Writes. Please add:
require.Equal(t, []any{
output.LogOutput{
Format: "%s/%s created",
Params: []any{"Applications.Test/exampleResources", "my-example"},
},
}, outputSink.Writes)| if err != nil { | ||
| // Proceed with registering manifests. Use a nil logger to suppress verbose | ||
| // progress messages; the concise success line is emitted below. | ||
| if err := manifest.RegisterFile(ctx, r.UCPClientFactory, "local", r.ResourceProviderManifestFilePath, nil); err != nil { |
There was a problem hiding this comment.
Two issues here:
(a) -o/--output flag is now silently ignored (same regression as resource/create/create.go). AddOutputFlag, RequireOutput, and r.Format = format are all still present, but WriteFormatted is gone. Either drop the flag wiring or restore conditional formatted output for non-default formats.
(b) Dead Logger field. Lines 79 and 88 still declare and initialize Runner.Logger, but you now pass literal nil here. The field is unreachable. Please either:
- remove the
Loggerfield and itsNewRunnerinitialization, or - pass
r.Loggerhere (but then the verbose progress messages that you were trying to silence will come back, so removing is probably what you want).
| err := manifest.EnsureResourceProviderExists(ctx, r.UCPClientFactory, defaultPlaneName, *r.ResourceProvider, r.Logger) | ||
| // Always ensure the resource provider exists first. Use a nil logger to suppress | ||
| // verbose progress messages; the concise success line is emitted below. | ||
| err := manifest.EnsureResourceProviderExists(ctx, r.UCPClientFactory, defaultPlaneName, *r.ResourceProvider, nil) |
There was a problem hiding this comment.
Dead Logger field and dead constants.
The Logger field (declared at line 92, initialized in NewRunner at line 100) is no longer used anywhere — the three manifest.* calls in this file (lines 162, 181) all pass literal nil. Same applies to pkg/cli/cmd/resourceprovider/create/create.go. Please remove the Logger field and its NewRunner setup.
Also, the constants msgNoResourceTypeNameProvided and msgAllResourceTypesCreated declared at lines 36–37 are no longer referenced anywhere. Please delete them.
And: the -o/--output flag is silently ignored on this command too (same regression as resource/create and resourceprovider/create).
| // Single type - success message already logged by RegisterType | ||
| } else { | ||
| r.Output.LogInfo(msgAllResourceTypesCreated) | ||
| r.Output.LogInfo("%s/%s created", r.ResourceProvider.Namespace, typeName) |
There was a problem hiding this comment.
Identifier format is inconsistent with other commands. Most messages in this PR use a fully-qualified ARM-style ID (e.g., Applications.Core/applications/foo deleted, System.Resources/resourceProviders/foo deleted), but here the format is just <provider-namespace>/<typeName> with no resourceTypes/ segment. For consistency consider:
r.Output.LogInfo("%s/resourceTypes/%s/%s created", r.ResourceProvider.Namespace, typeName) // or pick any other conventionThe key thing is to settle on one identifier format and apply it across this PR (this file, resourcetype/delete, workspace/create, and workspace/delete are the outliers).
|
|
||
| if deleted { | ||
| r.Output.LogInfo("Resource type %q deleted.", r.ResourceTypeName) | ||
| r.Output.LogInfo("%s deleted", r.ResourceTypeName) |
There was a problem hiding this comment.
Identifier format inconsistency. This emits just r.ResourceTypeName (which is <namespace>/<typeSuffix>, e.g., My.Provider/myType deleted), whereas the other delete commands prefix the resource provider, e.g., Applications.Core/applications/foo deleted, System.Resources/resourceProviders/foo deleted. Please pick one convention and apply consistently (see also the matching comment on resourcetype/create/create.go).
| return err | ||
| } | ||
| output.LogInfo("Set %q as current workspace", r.Workspace.Name) | ||
| r.Output.LogInfo("workspace/%s created", r.Workspace.Name) |
There was a problem hiding this comment.
Three issues on these two lines:
(a) Violates the issue's acceptance criteria. #11769 is explicit: "All rad <resource> create commands output exactly one line when the command is successful." Two lines are emitted here. Suggest collapsing to a single line, e.g.:
r.Output.LogInfo("workspace/%s created (current)", r.Workspace.Name)(b) Identifier format inconsistency. Other commands use the fully-qualified ARM-style ID (e.g., Applications.Core/applications/foo created, System.Resources/resourceGroups/foo created); this uses lowercase workspace/foo. Workspaces aren't ARM resources so a different format is defensible, but please pick one convention for the PR (see the related comments on resourcetype/{create,delete}/delete.go and workspace/delete/delete.go).
(c) Untested. pkg/cli/cmd/workspace/create/create_test.go was not modified in this PR and only checks require.NoError(t, err). The new log lines (and the removal of "Creating workspace..." and the package-level output.LogInfo(...) call) are not asserted. Please add:
require.Equal(t, []any{
output.LogOutput{Format: "workspace/%s created", Params: []any{"defaultWorkspace"}},
output.LogOutput{Format: "workspace/%s set as current", Params: []any{"defaultWorkspace"}},
}, outputSink.Writes)(or whatever single-line replacement you go with.)
| return err | ||
| } | ||
|
|
||
| r.Output.LogInfo("workspace/%s deleted", r.Workspace.Name) |
There was a problem hiding this comment.
Identifier format inconsistency. Same concern as on workspace/create/create.go — workspace/<name> differs from the <Provider>/<plural-type>/<name> convention used everywhere else in this PR. Please align with whatever single convention you settle on.
| } | ||
|
|
||
| r.Output.LogInfo("Successfully updated environment %q.", r.EnvName) | ||
| r.Output.LogInfo("Applications.Core/environments/%s updated", r.EnvName) |
There was a problem hiding this comment.
Orphan dead code in this package. Now that update.go no longer uses environmentForDisplay / environmentFormat, the file pkg/cli/cmd/env/update/objectformats.go is dead production code (the only remaining reference is objectformats_test.go, which exists solely to test that dead helper). The PR correctly deleted the parallel pkg/cli/cmd/env/update/preview/objectformats.go; please also delete pkg/cli/cmd/env/update/objectformats.go and its sibling objectformats_test.go for consistency.
| msgRecipePackNotFound = "Recipe pack %s does not exist or has already been deleted." | ||
| msgRecipePackNotDeleted = "Recipe pack %q NOT deleted" | ||
| msgRecipePackDeleted = "Radius.Core/recipePacks/%s deleted" | ||
| msgRecipePackNotDeleted = "Radius.Core/recipePacks/%s not deleted" |
There was a problem hiding this comment.
Inconsistent cancel-prompt behavior across commands. This file kept a msgRecipePackNotDeleted constant and still logs it when the user declines the prompt (line 141). Every other delete command in this PR — env/delete, group/delete, resource/delete, plus the previously-existing behavior of app/delete — was changed to return silently on ConfirmNo. So:
rad env delete(cancelled) → silentrad group delete(cancelled) → silentrad resource delete(cancelled) → silentrad recipe-pack delete(cancelled) →Radius.Core/recipePacks/foo not deleted
Please pick one behavior and apply it everywhere. Personally I'd lean toward keeping a short not deleted line on cancel for all of them — silent cancel can leave users wondering whether their n keypress did anything — but either choice is fine as long as it's consistent.
3 participants
This pull request standardizes and simplifies log messages across several CLI commands related to applications and environments. The changes focus on making log output more consistent, concise, and aligned with resource identifiers, while also cleaning up unnecessary progress or verbose messages. Corresponding test cases have been updated to match the new output formats.
Standardization of log messages:
Application delete commands now log messages using resource-style identifiers, such as
"Applications.Core/applications/%s deleted"and"Applications.Core/applications/%s not found", instead of more verbose or variable messages. [1] [2] [3] [4] [5] [6]Environment create, delete, and update commands (including preview variants) now use consistent messages like
"Applications.Core/environments/%s created","Applications.Core/environments/%s deleted","Applications.Core/environments/%s updated", and their"Radius.Core/environments/%s ..."equivalents for preview commands. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Removal of unnecessary or redundant log output:
Test updates for new log formats:
These changes improve the clarity and consistency of CLI output, making it easier for users to understand the results of their actions and for automated tools to parse log output.# Description
Type of change
Fixes: #11769
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: