Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: group create, get, list command #1849

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SafarMirek
Copy link
Contributor

This PR adds group command to service-registry part of cli tool. Group actions were added in service-registry API and this PR adds there features to the CLI.

Verification Steps

  1. Use rhoas service-registry group list to see list of existing groups
  2. Create new group using rhoas service-registry group create
  3. Use rhoas service-registry group list to see if group created in 2 is there
  4. Use rhoas service-registry group get to get metadata of the created group

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)

one = 'Group {{.GroupId}} was successfully created'

[group.cmd.create.input.group-id.message]
one = 'Name of the group:'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In every place besides the prompt we call this the group-id but here we call it the name. Is this meant to be like this?

Copy link
Contributor

@jackdelahunt jackdelahunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

if err != nil {
return registrycmdutil.TransformInstanceError(err)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return the message saying no groups available if rhoas service-registry group list returns no results.


flags.StringVarP(&opts.groupId, "group-id", "g", "", opts.f.Localizer.MustLocalize("group.cmd.create.flag.group-id"))
flags.StringVarP(&opts.description, "description", "d", "", opts.f.Localizer.MustLocalize("group.cmd.create.flag.description"))
flags.StringToStringVarP(&opts.properties, "properties", "p", map[string]string{}, opts.f.Localizer.MustLocalize("group.cmd.create.flag.properties"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--properties doesn't work as expected.

> ./rhoas service-registry group create --group-id rama-test-104 --properties "balance=100,max:90990" --description "[]" --instance-id 79fb0b01-cc23-4e06-81f4-47004b74211e
{
  "createdBy": "rpattnai",
  "createdOn": "2023-04-13T05:50:32Z",
  "description": "[]",
  "id": "rama-test-104",
  "modifiedBy": "",
  "modifiedOn": "1970-01-01T00:00:00Z",
  "properties": {
    "balance": "100,max:90990"
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIll suggest to check the implementation in connector cluster update command.

return nil
}

func runInteractivePrompt(opts *options, missingFlags []string) (err error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interactive mode allows --group-id to be "". Is this expected? Should we add some validations?

flags := rulecmdutil.NewFlagSet(cmd, f)

flags.StringVarP(&opts.groupId, "group-id", "g", "", opts.f.Localizer.MustLocalize("group.cmd.create.flag.group-id"))
flags.StringVarP(&opts.description, "description", "d", "", opts.f.Localizer.MustLocalize("group.cmd.create.flag.description"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't currently have shorthand for description and properties currently.

return dump.Formatted(opts.f.IOStreams.Out, opts.outputFormat, groupMetaData)
}

func runInteractivePrompt(opts *options, missingFlags []string) (err error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have interactive mode for get operations, it will be better to remove it.

flags := rulecmdutil.NewFlagSet(cmd, f)

flags.Int32VarP(&opts.page, "page", "", cmdutil.ConvertPageValueToInt32(build.DefaultPageNumber), opts.f.Localizer.MustLocalize("group.cmd.list.flag.page"))
flags.Int32VarP(&opts.limit, "limit", "", 100, opts.f.Localizer.MustLocalize("group.cmd.list.flag.limit"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason of keeoing 100 as limit here?

@@ -0,0 +1,100 @@
[group.cmd.description.short]
one = 'Manage artifacts groups of Service Registry instance'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
one = 'Manage artifacts groups of Service Registry instance'
one = 'Manage artifact groups of Service Registry instance'


[group.cmd.description.long]
one = '''
Manage artifacts groups of Service Registry instance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Manage artifacts groups of Service Registry instance
Manage the artifact groups of the current Service Registry instance


[group.cmd.example]
one = '''
## List all artifacts groups of the current Service Registry instance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## List all artifacts groups of the current Service Registry instance
## List all artifact groups of the current Service Registry instance

## List all artifacts groups of the current Service Registry instance
$ rhoas service-registry group list

## Get artifact group metadata by group-id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## Get artifact group metadata by group-id
## Get artifact group metadata by group ID


[group.list.cmd.description.long]
one = '''
List artifact groups of a Service Registry instance with their id and description
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
List artifact groups of a Service Registry instance with their id and description
List artifact groups of a Service Registry instance with their group ID and description

Copy link
Contributor

@smccarthy-ie smccarthy-ie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SafarMirek Looks good. Left a few minor suggestions for the help text, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants