WIP: feat(service-registry): setting command #1677
Conversation
SafarMirek
force-pushed
the
sr-setting-cmd
branch
2 times, most recently
from
6c3ae0a to
43a6019
Compare
Service registry setting command with subcommands: get, set, list
SafarMirek
force-pushed
the
sr-setting-cmd
branch
from
43a6019 to
a0f1ee1
Compare
|
|
||
| configProperty, _, err := request.Execute() | ||
| if err != nil { | ||
| return registrycmdutil.TransformInstanceError(err) |
There was a problem hiding this comment.
Nice! Really really nice that you have found and used that method.
|
|
||
| var missingFlags []string | ||
|
|
||
| if opts.settingName == "" { |
|
|
||
| opts.registryID = registryInstance.GetId() | ||
|
|
||
| return runSet(opts) |
There was a problem hiding this comment.
Reset to default would ignore other fields. We can:
- Return error
- Print warning that fields are ignored
- (current) do nothing and just reset it (but ignore values passed)
Your choice
There was a problem hiding this comment.
I have added the warning, that value is ignored, when default flag is present.
pkg/cmd/registry/setting/set/set.go
Outdated
|
|
||
| func runInteractivePrompt(opts *options, missingFlags []string) (err error) { | ||
|
|
||
| if slices.Contains(missingFlags, "setting-name") { |
There was a problem hiding this comment.
WOW :) very nice pattern :)
| [setting.set.cmd.example] | ||
| one = ''' | ||
| ## Set value of setting by name | ||
| $ rhoas service-registry setting set --setting-name registry.ccompat.legacy-id-mode.enabled --value true |
There was a problem hiding this comment.
Do we have some docs where those settings are documented etc. It might be good to include in long description
| } | ||
|
|
||
| if len(missingFlags) > 0 { | ||
| err = runInteractivePrompt(opts, missingFlags) |
There was a problem hiding this comment.
Very nice pattern to use interactive mode when flags are not set, I should use this more often 😄
|
|
||
| [setting.cmd.description.long] | ||
| one = ''' | ||
| Configure settings of a Service Registry instance |
There was a problem hiding this comment.
Maybe a longer description for this, it is the same for short and long
There was a problem hiding this comment.
We have dedicated documentation team that works on descriptions for the commands.
It is good to write something long but certainly not required.
There was a problem hiding this comment.
Hey folks,
This is an admin only-feature in the Apicurio Registry API and RHOSR UI. Assuming this is admin-only also in RHOAS CLI?
I can create a PR to update the help doc with more info (e.g. admin/instance owner feature, available settings, etc.).
There was a problem hiding this comment.
Good point. We might actually add some extra checks to see if user is admin (assuming that non admin would not get useful error.
|
lgtm 👍 |
|
@SafarMirek LGTM
Leaving for @jackdelahunt to leave any additional comments if needed. |
This PR adds new setting command group with three subcommands:
list : List current configuration settings
set : Set the value of a configuration setting
get: Get the value of a configuration setting
Closes #1650
Usage:
List all settings of the current Service Registry instance
Set the value of setting
Also, in registry there is an option to reset setting to default value, to do this using cli tool you can use a set command:
Get the calue of setting
From the original proposal I have added also get command to get ConfigurationProperty of specific name.
Type of change