-
Notifications
You must be signed in to change notification settings - Fork 41
Feat: Implementing mgrctl get system command. #789
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
base: main
Are you sure you want to change the base?
Changes from all commits
1e97b3d
da18b51
40b7efd
e0aab72
7584b1e
6f0a0dd
8ed8c60
701705d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| # SPDX-FileCopyrightText: 2023 SUSE LLC | ||
| # SPDX-FileCopyrightText: 2026 SUSE LLC | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
|
|
@@ -8,3 +8,4 @@ vendor.tar.gz | |
| __pycache__ | ||
| **/tags | ||
| *.mo | ||
| .DS_Store | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this should go in a separate commit? |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| // SPDX-FileCopyrightText: 2026 Jayprakash | ||
| // | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package get | ||
|
|
||
| import ( | ||
| "os" | ||
|
|
||
| "github.com/rs/zerolog/log" | ||
| "github.com/spf13/cobra" | ||
| "github.com/uyuni-project/uyuni-tools/shared/api" | ||
| . "github.com/uyuni-project/uyuni-tools/shared/l10n" | ||
| "github.com/uyuni-project/uyuni-tools/shared/types" | ||
| "github.com/uyuni-project/uyuni-tools/shared/utils" | ||
| ) | ||
|
|
||
| type getFlags struct { | ||
| api.ConnectionDetails `mapstructure:"api"` | ||
| OutputFormat string `mapstructure:"output"` | ||
| Filter string `mapstructure:"filter"` | ||
| Page int `mapstructure:"-"` | ||
| PageSize int `mapstructure:"-"` | ||
| } | ||
|
|
||
| func NewCommand(globalFlags *types.GlobalFlags) *cobra.Command { | ||
| var flags getFlags | ||
|
|
||
| cmd := &cobra.Command{ | ||
| Use: "get <resource-type> [flags]", | ||
| Short: L("Display one or many resources"), | ||
| Example: ` mgrctl get system | ||
| mgrctl get systemgroup | ||
| mgrctl get system --page 0 --page-size 10 | ||
| mgrctl get system --filter "extra_pkg_count>0"`, | ||
| Long: L(`Fetch and display Uyuni API resources in table, JSON, or YAML format. | ||
|
|
||
| The resource type is passed as the first argument. Filtering and pagination | ||
| are handled server-side when the API endpoint supports it. | ||
|
|
||
| Available resource types: | ||
| ` + GetResourceHelp() + ` | ||
|
|
||
| Filter Operators: | ||
| >=, <=, !=, =, >, < (e.g. extra_pkg_count>0) | ||
|
|
||
| Custom Columns: | ||
| Use -o custom-columns=HEADER:path,HEADER:path to extract specific JSON fields. | ||
| Example: -o custom-columns=ID:.id,NAME:.name`), | ||
| Args: cobra.MatchAll(cobra.ExactArgs(1), cobra.OnlyValidArgs), | ||
| ValidArgs: registeredTypes(), | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| return utils.CommandHelper(globalFlags, cmd, args, &flags, nil, runGet) | ||
| }, | ||
| } | ||
|
|
||
| utils.AddOutputFlag(cmd, &flags.OutputFormat) | ||
| cmd.Flags().StringVar(&flags.Filter, "filter", "", | ||
| L("Filter expression (e.g. extra_pkg_count=0)")) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the supported operators ( |
||
| 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")) | ||
|
Comment on lines
+60
to
+63
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure if we want these page flags
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| api.AddAPIFlags(cmd) | ||
| return cmd | ||
| } | ||
|
|
||
| func runGet(_ *types.GlobalFlags, flags *getFlags, _ *cobra.Command, args []string) error { | ||
| resourceType := args[0] | ||
| log.Debug().Msgf("Running get %s", resourceType) | ||
|
|
||
| fetcher, err := lookupFetcher(resourceType) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| client, err := api.Init(&flags.ConnectionDetails) | ||
| if err == nil && (client.Details.User != "" || client.Details.InSession) { | ||
| err = client.Login() | ||
| } | ||
| if err != nil { | ||
| return utils.Errorf(err, L("unable to login to the server")) | ||
| } | ||
|
|
||
| items, total, err := fetcher.List(client, flags.Filter, flags.Page, flags.PageSize) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if total > 0 && flags.PageSize > 0 { | ||
| log.Info().Msgf("Fetched %d items out of %d total", len(items), total) | ||
| } | ||
|
|
||
| return utils.PrintOutput(flags.OutputFormat, items, fetcher.Columns(), os.Stdout) | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,89 @@ | ||||||
| // SPDX-FileCopyrightText: 2026 Jayprakash | ||||||
| // | ||||||
| // SPDX-License-Identifier: Apache-2.0 | ||||||
|
|
||||||
| package get | ||||||
|
|
||||||
| import ( | ||||||
| "fmt" | ||||||
| "sort" | ||||||
| "strings" | ||||||
|
|
||||||
| "github.com/uyuni-project/uyuni-tools/shared/api" | ||||||
| apitypes "github.com/uyuni-project/uyuni-tools/shared/api/types" | ||||||
| . "github.com/uyuni-project/uyuni-tools/shared/l10n" | ||||||
| "github.com/uyuni-project/uyuni-tools/shared/utils" | ||||||
| ) | ||||||
|
|
||||||
| type ResourceFetcher interface { | ||||||
| List(client *api.APIClient, filter string, page, pageSize int) ([]map[string]any, int, error) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why returning a slice of maps? You should rather use the types that you define. It would later help with a formatting output type like |
||||||
| Columns() []utils.ColumnDef | ||||||
| } | ||||||
|
|
||||||
| type Resource struct { | ||||||
| Fetcher ResourceFetcher | ||||||
| Aliases []string | ||||||
| Description string | ||||||
| } | ||||||
|
|
||||||
| var resourceTypes = map[string]Resource{ | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be better to keep that variable empty here and add a |
||||||
| "system": { | ||||||
| Fetcher: systemFetcher{}, | ||||||
| Aliases: []string{"sys"}, | ||||||
| Description: "List systems", | ||||||
| }, | ||||||
|
Comment on lines
+30
to
+34
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be something like this in the |
||||||
| "systemgroup": { | ||||||
| Fetcher: systemGroupFetcher{}, | ||||||
| Aliases: []string{"grp"}, | ||||||
| Description: "List system groups", | ||||||
| }, | ||||||
| } | ||||||
|
|
||||||
| func registeredTypes() []string { | ||||||
| names := make([]string, 0) | ||||||
| for name, res := range resourceTypes { | ||||||
| names = append(names, name) | ||||||
| names = append(names, res.Aliases...) | ||||||
| } | ||||||
| sort.Strings(names) | ||||||
| return names | ||||||
| } | ||||||
|
|
||||||
| func registeredTypesText() string { | ||||||
| return strings.Join(registeredTypes(), ", ") | ||||||
| } | ||||||
|
|
||||||
| func GetResourceHelp() string { | ||||||
| var lines []string | ||||||
| for name, res := range resourceTypes { | ||||||
| aliases := "" | ||||||
| if len(res.Aliases) > 0 { | ||||||
| aliases = fmt.Sprintf(" (%s)", strings.Join(res.Aliases, ", ")) | ||||||
| } | ||||||
| lines = append(lines, fmt.Sprintf(" %-20s - %s", name+aliases, L(res.Description))) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
| } | ||||||
| sort.Strings(lines) | ||||||
| return strings.Join(lines, "\n") | ||||||
| } | ||||||
|
|
||||||
| func lookupFetcher(name string) (ResourceFetcher, error) { | ||||||
| if res, ok := resourceTypes[name]; ok { | ||||||
| return res.Fetcher, nil | ||||||
| } | ||||||
| for _, res := range resourceTypes { | ||||||
| if utils.Contains(res.Aliases, name) { | ||||||
| return res.Fetcher, nil | ||||||
| } | ||||||
| } | ||||||
| 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) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| res, err := api.Get[apitypes.FilteredResponse[map[string]any]]( | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
You should check if the endpoint exists instead of assuming it does. |
||||||
| client, fmt.Sprintf("%s?%s", endpoint, query), | ||||||
| ) | ||||||
| if err != nil { | ||||||
| return nil, 0, err | ||||||
| } | ||||||
| return res.Result.Data, res.Result.Total, nil | ||||||
| } | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There needs to be more unit tests, also ones for |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| // SPDX-FileCopyrightText: 2026 Jayprakash | ||
| // | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package get | ||
|
|
||
| import ( | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestResourceTypesNoDuplicates(t *testing.T) { | ||
| seen := make(map[string]bool) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for a map here, a slice is enough |
||
|
|
||
| for name, res := range resourceTypes { | ||
| if seen[name] { | ||
| t.Errorf("Duplicate resource key found: %s", name) | ||
| } | ||
| seen[name] = true | ||
|
|
||
| for _, alias := range res.Aliases { | ||
| if seen[alias] { | ||
| t.Errorf("Duplicate resource alias found: %s (in resource %s)", alias, name) | ||
| } | ||
| seen[alias] = true | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,49 @@ | ||||||
| // SPDX-FileCopyrightText: 2026 Jayprakash | ||||||
| // | ||||||
| // SPDX-License-Identifier: Apache-2.0 | ||||||
|
|
||||||
| package get | ||||||
|
|
||||||
| import ( | ||||||
| "fmt" | ||||||
| "net/url" | ||||||
| "strings" | ||||||
|
|
||||||
| "github.com/uyuni-project/uyuni-tools/shared/api" | ||||||
| "github.com/uyuni-project/uyuni-tools/shared/utils" | ||||||
| ) | ||||||
|
|
||||||
| type systemFetcher struct{} | ||||||
|
|
||||||
| func (systemFetcher) List(client *api.APIClient, filter string, page, pageSize int) ([]map[string]any, int, error) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| filterKey, filterValue := "", "" | ||||||
| if filter != "" { | ||||||
| filterKey, filterValue = parseFilter(filter) | ||||||
| } | ||||||
|
|
||||||
| query := url.Values{} | ||||||
| query.Set("filterKey", filterKey) | ||||||
| query.Set("filterValue", filterValue) | ||||||
| query.Set("page", fmt.Sprintf("%d", page)) | ||||||
| query.Set("pageSize", fmt.Sprintf("%d", pageSize)) | ||||||
|
|
||||||
| return listFiltered(client, "system/listSystemsFiltered", query.Encode()) | ||||||
|
marv7000 marked this conversation as resolved.
|
||||||
| } | ||||||
|
|
||||||
| func parseFilter(expr string) (string, string) { | ||||||
| for _, op := range []string{">=", "<=", "!=", "=", ">", "<"} { | ||||||
| if i := strings.Index(expr, op); i >= 0 { | ||||||
| return strings.TrimSpace(expr[:i]), strings.TrimSpace(expr[i:]) | ||||||
|
marv7000 marked this conversation as resolved.
|
||||||
| } | ||||||
| } | ||||||
| return strings.TrimSpace(expr), "" | ||||||
| } | ||||||
|
|
||||||
| func (systemFetcher) Columns() []utils.ColumnDef { | ||||||
| return []utils.ColumnDef{ | ||||||
| {Header: "ID", Field: "id"}, | ||||||
| {Header: "NAME", Field: "name"}, | ||||||
| {Header: "LAST_CHECKIN", Field: "last_checkin"}, | ||||||
| {Header: "CREATED", Field: "created"}, | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,29 @@ | ||||||
| // SPDX-FileCopyrightText: 2026 Jayprakash | ||||||
| // | ||||||
| // SPDX-License-Identifier: Apache-2.0 | ||||||
|
|
||||||
| package get | ||||||
|
|
||||||
| import ( | ||||||
| "github.com/uyuni-project/uyuni-tools/shared/api" | ||||||
| "github.com/uyuni-project/uyuni-tools/shared/utils" | ||||||
| ) | ||||||
|
|
||||||
| type systemGroupFetcher struct{} | ||||||
|
|
||||||
| func (systemGroupFetcher) List(client *api.APIClient, _ string, _, _ int) ([]map[string]any, int, error) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| res, err := api.Get[[]map[string]any](client, "systemgroup/listAllGroups") | ||||||
| if err != nil { | ||||||
| return nil, 0, err | ||||||
| } | ||||||
| return res.Result, len(res.Result), nil | ||||||
| } | ||||||
|
|
||||||
| func (systemGroupFetcher) Columns() []utils.ColumnDef { | ||||||
| return []utils.ColumnDef{ | ||||||
| {Header: "ID", Field: "id"}, | ||||||
| {Header: "NAME", Field: "name"}, | ||||||
| {Header: "DESCRIPTION", Field: "description"}, | ||||||
| {Header: "SYSTEM_COUNT", Field: "system_count"}, | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| // SPDX-FileCopyrightText: 2026 Jayprakash | ||
| // | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package types | ||
|
|
||
| // FilteredResponse describes the paginated response from filtered API endpoints | ||
| // like system/listSystemsFiltered. | ||
| type FilteredResponse[T any] struct { | ||
| Data []T `json:"data"` | ||
| Total int `json:"total"` | ||
| Page int `json:"page"` | ||
| PageSize int `json:"pageSize"` | ||
| } | ||
|
|
||
| // SystemGroup describes an Uyuni system group in the API. | ||
| type SystemGroup struct { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This struct seems unused? |
||
| ID int `json:"id" mapstructure:"id"` | ||
| Name string `json:"name" mapstructure:"name"` | ||
| Description string `json:"description" mapstructure:"description"` | ||
| OrgID int `json:"org_id" mapstructure:"org_id"` | ||
| SystemCount int `json:"system_count" mapstructure:"system_count"` | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| // SPDX-FileCopyrightText: 2026 SUSE LLC | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. copyright needs to be adjusted, since you created this one
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, I think this is the comment Marvin sir was referring to about the unused struct. I've already addressed that by removing the file entirely from the codebase in my upcoming commit to keep the PR clean. |
||
| // | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package types | ||
|
|
||
| // System describes an Uyuni registered system/minion in the API. | ||
| type System struct { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This struct seems to be never used? Is this intended? |
||
| ID int `json:"id" mapstructure:"id"` | ||
| Name string `json:"name" mapstructure:"name"` | ||
| LastCheckin string `json:"last_checkin" mapstructure:"last_checkin"` | ||
| Created string `json:"created" mapstructure:"created"` | ||
| LastBoot string `json:"last_boot" mapstructure:"last_boot"` | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,10 +37,10 @@ func DiffStrings(expected string, actual string) string { | |
|
|
||
| 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are you changing this? |
||
| } | ||
| if i < len(actualLines) { | ||
| diff.WriteString(fmt.Sprintf("+%d: %s\n", i+1, act)) | ||
| fmt.Fprintf(&diff, "+%d: %s\n", i+1, act) | ||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is fine, since you're changing an already existing file ;)