feat(service-registry rule): add describe and list command by rkpattnaik780 · Pull Request #1431 · redhat-developer/app-services-cli

rkpattnaik780 · 2022-02-14T08:33:15Z

Add describe and list command for service-registry rules.

Fetching global compatibility rule for artifacts in the Service Registry instance
{
  "config": "FULL",
  "type": "COMPATIBILITY"
}

Fetching global rules for artifacts in Service Registry Instance

  RULE TYPE       DESCRIPTION                                                 STATUS    
 --------------- ----------------------------------------------------------- ---------- 
  VALIDITY        Ensure that content is valid when updating this artifact.   DISABLED  
  COMPATIBILITY   Enforce a compatibility level when updating this            ENABLED   
                  artifact (for example, Backwards Compatibility).                      

Run describe command to view confiuration of enabled rule

Verification Steps

Run the following commands for list operation:

rhoas service-registry rule list

rhoas service-registry rule list --artifact-id my-id

Run following commands for describe operation.

./rhoas service-registry rule describe --rule-type compatibility

./rhoas service-registry rule describe --rule-type compatibility --artifact-id my-artifact

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation change
Other (please specify)

Checklist

Documentation added for the feature
CI and all relevant tests are passing
Code Review completed
Verified independently by reviewer

craicoverflow · 2022-02-14T10:26:19Z

Add describe and list command for service-registry rules.

It is better to not use screenshots unless necessary or to complement readable text. Screenshots cannot be understood by screen readers and so it makes this inaccessible to those with visual impairments. It is better to paste the code if needed.

Edit: though I see that this is not verification steps or anything important, so it is probably fine :)

craicoverflow · 2022-02-14T10:21:48Z

pkg/cmd/registry/rule/list/list.go

+var compatibilityRuleStatus = ruleRow{
+	RuleType:    ruleCompatibility,
+	Description: "Enforce a compatibility level when updating this artifact (for example, Backwards Compatibility).",
+	Status:      "DISABLED",


Suggested change

Status: "DISABLED",

Status: DISABLED_RULE_CONSTANT,

You should use a constant here.

craicoverflow · 2022-02-14T10:22:39Z

pkg/cmd/registry/rule/list/list.go

+
+var compatibilityRuleStatus = ruleRow{
+	RuleType:    ruleCompatibility,
+	Description: "Enforce a compatibility level when updating this artifact (for example, Backwards Compatibility).",


Should that use i18n?

craicoverflow · 2022-02-14T10:23:15Z

pkg/cmd/registry/rule/list/list.go

+
+	for _, rule := range enabledRules {
+		if rule == ruleValidity {
+			validityRuleStatus.Status = "ENABLED"


Suggested change

validityRuleStatus.Status = "ENABLED"

validityRuleStatus.Status = SHOULD_BE_A_CONSTANT

Would lowercase "enabled" be possible instead? Will this value ever need to be input by the user? If so we will want it to be lowercase.

It is just for display.

Even if it is just for display - all uppercase is harder to read than all lowercase.

Changed:

Fetching global rules for artifacts in Service Registry Instance RULE TYPE DESCRIPTION STATUS --------------- ----------------------------------------------------------- --------- validity Ensure that content is valid when updating this artifact. enabled compatibility Enforce a compatibility level when updating this enabled artifact (for example, Backwards Compatibility). Run describe command to view configuration of an enabled rule: $ rhoas service-registry rule describe --rule-type <compatibility/validity>

pkg/cmd/registry/rule/list/list.go

craicoverflow · 2022-02-14T10:31:20Z

pkg/cmd/registry/rule/list/list.go

+	ruleValidity      = "VALIDITY"
+	ruleCompatibility = "COMPATIBILITY"


Should these be in lowercase?

These serve two purpose: check the server response, display the status of rules.

craicoverflow · 2022-02-14T10:41:04Z

pkg/core/localize/locales/en/cmd/rule.en.toml

+one = 'No rules enabled'
+
+[registry.rule.list.log.info.describeHint]
+one = 'Run describe command to view confiuration of enabled rule'


Suggested change

one = 'Run describe command to view confiuration of enabled rule'

one = 'Run describe command to view configuration of enabled rule'

I think we should be less vague about what the describe command is, and just give them the full command instead.

Run "rhoas service-registry describe"

Changing text to :

Run describe command to view configuration of an enabled rule: $ rhoas service-registry rule describe --rule-type <compatibility/validity>

craicoverflow · 2022-02-14T11:47:39Z

pkg/cmd/registry/rule/describe/describe.go

@@ -0,0 +1,175 @@
+package describe


$ rhoas service-registry rule describe --rule-type compatibilitkiy -v ❌ Invalid value "&{0xc00044fd10}" for --rule-type, valid options are: "validity", "compatibility". Run the command in verbose mode using the -v flag to see more information

Putting in an invalid type-type value is not handled correctly.

pkg/cmd/registry/rule/enable/enable.go

rkpattnaik780 · 2022-02-14T13:23:37Z

pkg/cmd/registry/rule/enable/enable.go

 		return err
 	}

+	instanceIDPrompt := &survey.Input{


@wtrocki prompts for few flags got missed in the last PR.

I don't think we should be prompting for the instance ID - it is not a pattern used in the other commands.

wtrocki · 2022-02-15T09:53:38Z

pkg/cmd/registry/rule/enable/enable.go

+				}
+			}
+
+			cfg, err := opts.Config.Load()


This can be moved to line 101 - it is out of the place

craicoverflow

Verified functionally, good job 👍🏻

My main concern though is that the artifact ID and instance ID are not being validated in interactive mode. Can we have that?

craicoverflow · 2022-02-15T10:21:44Z

pkg/core/localize/locales/en/cmd/rule.en.toml

+
+[registry.rule.list.log.info.describeHint]
+one = '''
+Run describe command to view configuration of an enabled rule:


I don't think it reads very well currently, maybe this?

Suggested change

Run describe command to view configuration of an enabled rule:

Run the following to view the configuration of an enabled rule:

craicoverflow · 2022-02-15T10:22:07Z

pkg/core/localize/locales/en/cmd/rule.en.toml

+one = '''
+Run describe command to view configuration of an enabled rule:
+
+$ rhoas service-registry rule describe --rule-type <compatibility/validity>


This should be indented.

craicoverflow · 2022-02-15T10:47:42Z

pkg/cmd/registry/rule/enable/enable.go

 		return err
 	}

+	instanceIDPrompt := &survey.Input{


I don't think we should be prompting for the instance ID - it is not a pattern used in the other commands.

craicoverflow · 2022-02-15T10:49:22Z

pkg/cmd/registry/rule/enable/enable.go

+		return err
+	}
+
+	artifactIDPrompt := &survey.Input{


I think this would make more sense to be a list you can select from - it would be less error prone?

Validating and suggesting values becomes bit tricky here as artifact operations need the group before hand. Even if we put the group prompt before artifact prompt, there doesn't seem to be any way to retrieve the list of groups.

I don't think we should be prompting for the instance ID - it is not a pattern used in the other commands.

I agree but other interactive modes didn't have the instance id flags, I am not sure if we should make the interactive mode have less options than non-interactive.

Let's keep in mind that sometimes reasons why integration is soo hard is because API is simply making it hard. If there is some suggestion we can put for future versions of the API (this area is being worked on) it would be good for entire service.

Changed instance ID argument to flag only, removed from prompt.

craicoverflow · 2022-02-15T10:51:00Z

pkg/cmd/registry/rule/enable/enable.go

+		}
+	} else {
+
+		opts.Logger.Info(opts.localizer.MustLocalize(


This should have a spinner in front of it (use these as much as possible when some action is taking place)

pkg/cmd/registry/rule/enable/enable.go

craicoverflow · 2022-02-15T10:53:54Z

pkg/cmd/registry/rule/list/list.go

+
+	opts.localizer.MustLocalize("registry.rule.list.compatibilityRule.description")
+
+	var compatibilityRuleStatus = ruleRow{


Suggested change

var compatibilityRuleStatus = ruleRow{

compatibilityRuleStatus := ruleRow{

^ recommended

craicoverflow · 2022-02-15T10:54:35Z

pkg/cmd/registry/rule/list/list.go

+	RuleValidity      = "validity"
+	RuleCompatibility = "compatibility"
+)
+
+const (
+	RuleDisabled = "disabled"
+	RuleEnabled  = "enabled"


Do these variables need to be public?

craicoverflow · 2022-02-15T10:56:31Z

pkg/cmd/registry/rule/rulecmdutil/error.go

+	"github.com/redhat-developer/app-services-cli/pkg/core/localize"
+)
+
+type RegistryRuleError struct {


Suggested change

type RegistryRuleError struct {

type RegistryRuleError struct {

That name is misleading - when I saw it being used in the code I thought that it is an error struct.

craicoverflow · 2022-02-15T10:57:43Z

pkg/cmd/registry/rule/rulecmdutil/flagset.go

+// AddArtifactID adds a flag for setting the artifact ID
+func (fs *flagSet) AddArtifactID(artifactID *string) {
+	flagName := "artifact-id"
+
+	fs.StringVar(
+		artifactID,
+		flagName,
+		"",
+		fs.factory.Localizer.MustLocalize("artifact.common.id"),
+	)
+
+}


Could that be more common - isn't artifact-id used elsewhere?

flagbuilders for --artifact-id and --group will be moved to the registrycmdutil in the next PR.

craicoverflow

LGTM. However I recommend improving how the spinner indicator gets displayed by keeping the message there and not having it disappear.

craicoverflow · 2022-02-16T11:36:33Z

pkg/cmd/registry/rule/list/list.go

+		s.Stop()
+	} else {
+		s := spinner.New(opts.IO.ErrOut, opts.localizer)
+		s.SetLocalizedSuffix("registry.rule.list.log.info.fetching.artifactRules")


"Fetching global rules for artifacts in Service Registry Instance" disappears once the data has loaded Do you want that? Maybe it is better to not have any message at all since I am not sure if it tells the user something they don't know.

Otherwise it would be better to let that message remain on the top of the display even after it has completed.

Same goes in other instances where this happens.

Same goes in other instances where this happens.

Is this suggestion for list command only or all commands that use spinners?
I cant form a strong opinion about the loader text. We can probably remove it as it shows nothing new.
Cc @wtrocki

The loader text is fine but it should not just flash up for a half second and disappear - the user will be thinking they missed something important. It is best to ensure it just does not be deleted so the user can still see the message as normal.

Sounds good. Updating PR to keep the spinners.

Persisting the spinner texts runs to newline issue, logger.info() or \n for spacing doesn't seem to fix this either.

⣷ Fetching rules for the artifact❌ Artifact with ID 41590799-c99f-47cc-85a2-c50578 not found. Run the command in verbose mode using the -v flag to see more information

Any instance where we have leave spinner unclosed?

…1431)" This reverts commit 93feb7b.

rkpattnaik780 changed the base branch from main to service-registry-rules February 14, 2022 08:33

rkpattnaik780 requested a review from a team as a code owner February 14, 2022 08:33

rkpattnaik780 requested a review from bredamc February 14, 2022 10:34

rkpattnaik780 changed the title ~~Rules view~~ feat(service-registry rule): add describe and list command Feb 14, 2022

craicoverflow suggested changes Feb 14, 2022

View reviewed changes

rkpattnaik780 commented Feb 14, 2022

View reviewed changes

pkg/cmd/registry/rule/enable/enable.go Show resolved Hide resolved

rkpattnaik780 commented Feb 14, 2022

View reviewed changes

rkpattnaik780 requested review from craicoverflow and wtrocki February 15, 2022 09:00

craicoverflow force-pushed the rules_view branch from 50db4d8 to e5a715e Compare February 15, 2022 09:35

craicoverflow force-pushed the service-registry-rules branch from a57c9b1 to 6a13f3e Compare February 15, 2022 09:37

craicoverflow force-pushed the rules_view branch from e5a715e to 8cd92aa Compare February 15, 2022 09:43

wtrocki reviewed Feb 15, 2022

View reviewed changes

rkpattnaik780 force-pushed the rules_view branch from 8cd92aa to 8dbca10 Compare February 15, 2022 10:08

craicoverflow reviewed Feb 15, 2022

View reviewed changes

rkpattnaik780 requested a review from craicoverflow February 15, 2022 14:17

rkpattnaik780 added 12 commits February 16, 2022 16:58

feat(service-registry rule): add list and describe command

84be38d

docs: change command description

f21e2d3

refactor: removed unused error handlers

be5291c

fix: error message for invalid rule type

4e3ddd7

fix(rule enable): check flags in non interactive mode

1e5dde9

fix: address review comments

4c46784

fix: prompts for interactive mode

1d83794

fix: change table cells to lowercase

5f24dbe

fix: address review comments

81a0bb0

fix: remove unused i18n keys

e989b4e

fix: typo and unused variables

86f7ff2

fix: mssing spinner stops

89d35b5

rkpattnaik780 force-pushed the rules_view branch from 82d4ff0 to 89d35b5 Compare February 16, 2022 11:32

craicoverflow approved these changes Feb 16, 2022

View reviewed changes

rkpattnaik780 merged commit f3219bd into service-registry-rules Feb 16, 2022

rkpattnaik780 deleted the rules_view branch February 16, 2022 12:51

rkpattnaik780 added a commit that referenced this pull request Feb 21, 2022

feat(service-registry rule): add describe and list command (#1431)

93feb7b

rkpattnaik780 added a commit that referenced this pull request Feb 21, 2022

Revert "feat(service-registry rule): add describe and list command (#…

a4b1bef

…1431)" This reverts commit 93feb7b.

	validityRuleStatus.Status = "ENABLED"
	validityRuleStatus.Status = SHOULD_BE_A_CONSTANT

		ruleValidity = "VALIDITY"
		ruleCompatibility = "COMPATIBILITY"

	one = 'Run describe command to view confiuration of enabled rule'
	one = 'Run describe command to view configuration of enabled rule'

	Run describe command to view configuration of an enabled rule:
	Run the following to view the configuration of an enabled rule:


		opts.localizer.MustLocalize("registry.rule.list.compatibilityRule.description")

		var compatibilityRuleStatus = ruleRow{

	var compatibilityRuleStatus = ruleRow{
	compatibilityRuleStatus := ruleRow{

	type RegistryRuleError struct {
	type RegistryRuleError struct {

Conversation

rkpattnaik780 commented Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Verification Steps

Type of change

Checklist

Uh oh!

craicoverflow commented Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craicoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craicoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rkpattnaik780 commented Feb 14, 2022 •

edited

Loading

craicoverflow commented Feb 14, 2022 •

edited

Loading