Feat: Implementing mgrctl get system command. by katara-Jayprakash · Pull Request #789 · uyuni-project/uyuni-tools

katara-Jayprakash · 2026-05-11T15:55:44Z

What does this PR change?

This PR adds an initial get command group to mgrctl and introduces mgrctl get system for listing registered systems from
the Uyuni API.

Included in this PR

Adds mgrctl get as a new top-level command group
Adds mgrctl get system (alias: systems) using system/listSystems
Supports output formats:
- table (default)
- json
- yaml
Adds API type mapping for system responses
Adds unit test coverage for mgrctl get system with mocked API login + listSystems flow

Codespace

Check if you already have a running container clicking on

Test coverage

Unit tests were added
DONE

Links

Issue(s): #

DONE

Changelogs

Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository

If you don't need a changelog check, please mark this checkbox:

No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Before you merge

Check How to branch and merge properly!

katara-Jayprakash · 2026-05-11T16:25:15Z

/cc @cbosdo @marv7000 @mbussolotto

Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>

sonarqubecloud · 2026-06-09T14:04:36Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>

cbosdo · 2026-06-30T13:28:39Z

+			filterKey := parts[0]
+			filterValue := parts[1]
+			// The API strictly requires filterKey, filterValue, page, and pageSize
+			url := fmt.Sprintf("system/listSystemsFiltered?filterKey=%s&filterValue=%s&page=0&pageSize=50", filterKey, filterValue)


There are several issues:

you only get the first 50 items

Passing the user-provided values as is is probably going to fail

cbosdo · 2026-06-30T14:17:46Z

 		if exp != act {
 			if i < len(expectedLines) {
-				diff.WriteString(fmt.Sprintf("-%d: %s\n", i+1, exp))
+				fmt.Fprintf(&diff, "-%d: %s\n", i+1, exp)


Why are you changing this?

cbosdo · 2026-06-30T14:28:10Z

@@ -0,0 +1,85 @@
+// SPDX-FileCopyrightText: 2026 SUSE LLC


Copyright is not for SUSE here, it's yours

cbosdo · 2026-06-30T14:30:43Z

+func NewCommand(globalFlags *types.GlobalFlags) *cobra.Command {
+	var flags getFlags
+
+	// 1. Parse the first word ("system") as a positional argument


Suggested change

// 1. Parse the first word ("system") as a positional argument

// Parse the first word ("system") as a positional argument

Please don't add the steps numbers: it's a pain to maintain

cbosdo · 2026-06-30T14:48:00Z

+	query.Set("filterKey", filterKey)
+	query.Set("filterValue", filterValue)
+	query.Set("page", fmt.Sprintf("%d", page))
+	query.Set("pageSize", fmt.Sprintf("%d", pageSize))


These are specific to the system: move them to the systemFetcher

cbosdo · 2026-06-30T14:50:44Z

+	// Set up the URL parameters (like adding ?page=1&pageSize=50 to a URL)
+	query := url.Values{}
+	query.Set("filterKey", filterKey)
+	query.Set("filterValue", filterValue)


the filterKey and filterValue probably need to be sanitized to prevent the user from breaking the HTTP query

cbosdo · 2026-06-30T14:52:34Z

+// struct that has these two exact functions:
+type ResourceFetcher interface {
+	// A function that knows how to fetch the data from the API
+	List(client *api.APIClient, filter string, page, pageSize int) ([]map[string]any, error)


List also needs to return the total number of items as it may not match the length of the returned slice if paging is performed.

cbosdo · 2026-06-30T14:55:44Z

+// 3. This is the "dictionary" that powers the whole architecture.
+// When the user types 'mgrctl get system', we look up the word "system" here.
+// To add a new command in the future, you just add ONE line to this map.
+var resourceTypes = map[string]ResourceFetcher{


You probably want to also have short names or aliases for the resources. Of course a unit test should check that there is no duplicate too.

so system could be shortened to sys and systemgroup to grp.

This variable needs to be looped through to show all the possible names and aliases in the help.

cbosdo · 2026-06-30T14:56:47Z

+	. "github.com/uyuni-project/uyuni-tools/shared/l10n"
+)
+
+// 1. This tells the system how to map a column header (like "ID")


Numbered comments are a pain to maintain…

cbosdo · 2026-06-30T14:59:27Z

+	fetcher, ok := resourceTypes[name]
+	if !ok {
+		// If the user typed something wrong (like 'mgrctl get fake'), it throws an error.
+		return nil, fmt.Errorf(L("unknown resource type %q; available: %s"), name, registeredTypesText())


The list of resource names and aliases can grow big over time. Better list them in a nice way in the help and point to the help from the errors.

cbosdo · 2026-06-30T15:00:28Z

+		return nil, err
+	}
+	// Return the raw JSON data
+	return res.Result.Data, nil


Return the total items count for paging too.

cbosdo · 2026-06-30T15:03:11Z

+	}
+
+	// 2. Parse the output and filter flags and save them to the getFlags struct
+	cmd.Flags().StringVarP(&flags.OutputFormat, "output", "o", "table",


The output format flag definition should be in a function in the shared/utils module in order to be shared too.

cbosdo · 2026-06-30T15:05:08Z

+
+	// 2. Parse the output and filter flags and save them to the getFlags struct
+	cmd.Flags().StringVarP(&flags.OutputFormat, "output", "o", "table",
+		L("Output format: table|json|yaml|custom-columns=SPEC|custom-columns-file=PATH"))


In the help, document what the columns spec needs to be: users can't guess

cbosdo · 2026-06-30T15:06:48Z

+	cmd.Flags().StringVarP(&flags.OutputFormat, "output", "o", "table",
+		L("Output format: table|json|yaml|custom-columns=SPEC|custom-columns-file=PATH"))
+	cmd.Flags().StringVar(&flags.Filter, "filter", "",
+		L("Filter expression (e.g. extra_pkg_count=0)"))


You need to at least document what are the possible operators. For the keys, it's a bit more complex as it is tied to the API and each object, but it would be nice to list them exhaustively too. I only fear it could be hard to maintain.

cbosdo · 2026-06-30T15:08:01Z

+	// A function that knows how to fetch the data from the API
+	List(client *api.APIClient, filter string, page, pageSize int) ([]map[string]any, error)
+	// A function that returns the default table columns
+	Columns() []ColumnDef


Add a function to return the help dedicated to this resource. This way you can loop over them in the command help in a special section about the supported resources.

cbosdo · 2026-06-30T15:10:26Z

+// When the user types 'mgrctl get system', we look up the word "system" here.
+// To add a new command in the future, you just add ONE line to this map.
+var resourceTypes = map[string]ResourceFetcher{
+	"system":      systemFetcher{},


With a Resource type, you could extend this to aliases.

Suggested change

"system": systemFetcher{},

"system": Resource{ []string{"sys"}, systemFetcher{} },

Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>

sonarqubecloud · 2026-07-04T13:04:30Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

marv7000 · 2026-07-06T11:17:36Z

+	return nil, fmt.Errorf(L("unknown resource type %[1]q; available: %[2]s"), name, registeredTypesText())
+}
+
+func listFiltered(client *api.APIClient, endpoint, query string) ([]map[string]any, int, error) {


listFiltered doesn't make sense to exist if you're passing the endpoint and query parameters as function arguments, only to assemble it there.

marv7000 · 2026-07-06T11:37:39Z

+}
+
+func listFiltered(client *api.APIClient, endpoint, query string) ([]map[string]any, int, error) {
+	res, err := api.Get[apitypes.FilteredResponse[map[string]any]](


Suggested change

res, err := api.Get[apitypes.FilteredResponse[map[string]any]](

res, err := api.GetChecked[apitypes.FilteredResponse[map[string]any]](

You should check if the endpoint exists instead of assuming it does.

marv7000 · 2026-07-06T11:40:38Z

+package types
+
+// System describes an Uyuni registered system/minion in the API.
+type System struct {


This struct seems to be never used? Is this intended?

marv7000 · 2026-07-06T11:40:54Z

+}
+
+// SystemGroup describes an Uyuni system group in the API.
+type SystemGroup struct {


This struct seems unused?

marv7000 · 2026-07-06T11:44:25Z

 __pycache__
 **/tags
 *.mo
+.DS_Store


Maybe this should go in a separate commit?

marv7000 · 2026-07-06T11:48:09Z

There needs to be more unit tests, also ones for parseFilter, runGet and printTable.

mbussolotto · 2026-07-07T07:12:22Z

+	cmd.Flags().IntVar(&flags.Page, "page", 0,
+		L("Page number for paginated results (0-indexed)"))
+	cmd.Flags().IntVar(&flags.PageSize, "page-size", 50,
+		L("Number of items per page"))


not sure if we want these page flags

At least it delegates the complex part of getting all the results to the user and his scripts. Looping is nice but you can get incoherent results if a change happens in the middle. I would keep them and state in their help that they may not be used, depending on the resource type and its corresponding API.

cbosdo · 2026-07-07T07:49:56Z

+	cmd.Flags().IntVar(&flags.Page, "page", 0,
+		L("Page number for paginated results (0-indexed)"))
+	cmd.Flags().IntVar(&flags.PageSize, "page-size", 50,
+		L("Number of items per page"))


At least it delegates the complex part of getting all the results to the user and his scripts. Looping is nice but you can get incoherent results if a change happens in the middle. I would keep them and state in their help that they may not be used, depending on the resource type and its corresponding API.

cbosdo · 2026-07-07T07:56:38Z

+	Description string
+}
+
+var resourceTypes = map[string]Resource{


It would be better to keep that variable empty here and add a registerResource() funtion to be called in each resource file.

cbosdo · 2026-07-07T07:58:15Z

+	"system": {
+		Fetcher:     systemFetcher{},
+		Aliases:     []string{"sys"},
+		Description: "List systems",
+	},


This should be something like this in the system.go file:

registerResource("system", systemFetcher{}, []string{"sys"}, "List systems")

cbosdo · 2026-07-07T08:00:52Z

+)
+
+func TestResourceTypesNoDuplicates(t *testing.T) {
+	seen := make(map[string]bool)


No need for a map here, a slice is enough

cbosdo · 2026-07-07T08:07:49Z

+
+type systemGroupFetcher struct{}
+
+func (systemGroupFetcher) List(client *api.APIClient, _ string, _, _ int) ([]map[string]any, int, error) {


Suggested change

func (systemGroupFetcher) List(client *api.APIClient, _ string, _, _ int) ([]map[string]any, int, error) {

func (systemGroupFetcher) List(client *api.APIClient, _ string, _, _ int) ([]SystemGroup, int, error) {

cbosdo · 2026-07-07T08:08:06Z

+
+type systemFetcher struct{}
+
+func (systemFetcher) List(client *api.APIClient, filter string, page, pageSize int) ([]map[string]any, int, error) {


Suggested change

func (systemFetcher) List(client *api.APIClient, filter string, page, pageSize int) ([]map[string]any, int, error) {

func (systemFetcher) List(client *api.APIClient, filter string, page, pageSize int) ([]System, int, error) {

cbosdo · 2026-07-07T08:09:55Z

+
+func AddOutputFlag(cmd *cobra.Command, outputFormat *string) {
+	cmd.Flags().StringVarP(outputFormat, "output", "o", "table",
+		L("Output format: table|json|yaml|custom-columns=SPEC|custom-columns-file=PATH"))


I am pestering often enough against kubectl get help linking to web pages for the format description: you need to add at least a rough description of what the SPEC is for that format.

cbosdo · 2026-07-07T08:13:44Z

+		return printTable(items, parsed, out)
+	}
+
+	if strings.HasPrefix(format, "custom-columns-file=") {


This should just be a wrapper around the custom-columns implementation rather than a separate implementation. Why are those two cases also not in the switch below? This made me think that you forgot them…

I would rather see them as printColumns() and printColumnsFromFile() function where the code is shared there.

cbosdo · 2026-07-07T08:16:46Z

+	return cols
+}
+
+func parseCustomColumnsFile(path string) ([]ColumnDef, error) {


This function should only parse the file into the format for the custom-columns. Then you would have the logic to create the ColumnDef in one place only.

katara-Jayprakash added 5 commits June 9, 2026 19:32

feat: Add mgrctl get system command prototype

1e97b3d

added test for system_test.go namespace

da18b51

Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>

fix: govulncheck -tags ptf and golangci-lint

40b7efd

Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>

chore: trigger CI rerun

e0aab72

Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>

working on something

7584b1e

Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>

katara-Jayprakash force-pushed the feat/mgrctl-get-system-command branch from 4f67d97 to 7584b1e Compare June 9, 2026 14:04

feat: add filter flag and custom columns support to get system command

6f0a0dd

Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>

katara-Jayprakash force-pushed the feat/mgrctl-get-system-command branch from 6f0a0dd to 9b288c7 Compare June 23, 2026 18:20

Refactor get command

8ed8c60

Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>

cbosdo reviewed Jun 30, 2026

View reviewed changes

katara-Jayprakash force-pushed the feat/mgrctl-get-system-command branch from 9b288c7 to 1a71128 Compare June 30, 2026 13:34

cbosdo reviewed Jun 30, 2026

View reviewed changes

mbussolotto self-requested a review June 30, 2026 14:49

cbosdo reviewed Jun 30, 2026

View reviewed changes

katara-Jayprakash force-pushed the feat/mgrctl-get-system-command branch 3 times, most recently from 7e3f8f5 to ce72c46 Compare July 4, 2026 13:02

Adding comments

701705d

Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>

katara-Jayprakash force-pushed the feat/mgrctl-get-system-command branch from ce72c46 to 701705d Compare July 4, 2026 13:04

marv7000 reviewed Jul 6, 2026

View reviewed changes

mbussolotto reviewed Jul 7, 2026

View reviewed changes

cbosdo reviewed Jul 7, 2026

View reviewed changes

	// 1. Parse the first word ("system") as a positional argument
	// Parse the first word ("system") as a positional argument

	"system": systemFetcher{},
	"system": Resource{ []string{"sys"}, systemFetcher{} },

	res, err := api.Get[apitypes.FilteredResponse[map[string]any]](
	res, err := api.GetChecked[apitypes.FilteredResponse[map[string]any]](


		type systemGroupFetcher struct{}

		func (systemGroupFetcher) List(client *api.APIClient, _ string, _, _ int) ([]map[string]any, int, error) {


		type systemFetcher struct{}

		func (systemFetcher) List(client *api.APIClient, filter string, page, pageSize int) ([]map[string]any, int, error) {

Uh oh!

Conversation

katara-Jayprakash commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR change?

Included in this PR

Codespace

Test coverage

Links

Changelogs

Before you merge

Uh oh!

katara-Jayprakash commented May 11, 2026

Uh oh!

sonarqubecloud Bot commented Jun 9, 2026

Quality Gate passed

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!

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!

sonarqubecloud Bot commented Jul 4, 2026

Quality Gate passed

Uh oh!

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!

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

katara-Jayprakash commented May 11, 2026 •

edited

Loading