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

incus/cluster: Add customizable columns to list #784

Merged
merged 2 commits into from
Apr 24, 2024
Merged

Conversation

Anish-M
Copy link
Contributor

@Anish-M Anish-M commented Apr 23, 2024

Add customizable columns to incus cluster list

@Anish-M Anish-M requested a review from stgraber as a code owner April 23, 2024 21:11
@stgraber stgraber changed the title issue 721 incus cluster list incus/cluster: Add customizable columns to list Apr 24, 2024
@@ -19,10 +19,17 @@ import (
"github.com/lxc/incus/v6/shared/util"
)


Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant empty line.

type cmdCluster struct {
global *cmdGlobal
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant empty line.

flagFormat string
flagAllProjects bool

Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant empty line.

// Show.
type cmdClusterShow struct {
global *cmdGlobal
cluster *cmdCluster

Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant empty line.

return cli.RenderTable(c.flagFormat, header, data, members)
}



Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant empty lines.

@@ -292,6 +406,7 @@ func (c *cmdClusterInfo) Run(cmd *cobra.Command, args []string) error {
return err
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant empty line.


for _, columnRune := range columnEntry {
column, ok := columnsShorthandMap[columnRune]
if ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can simplify this if..else:

if ok {
    columns = append(columns, column)
}

return nil, fmt.Errorf(i18n.G("Unknown column shorthand char '%c' in '%s'"), columnRune, columnEntry)

Anish-M and others added 2 commits April 24, 2024 23:01
Closes lxc#721

Signed-off-by: Anish Mankal <anish.mankal@gmail.com>
Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
@presztak
Copy link
Contributor

@Anish-M I've added couple of minor comments. In addition:

  1. You need to add Signed-off-by to the commit. You can do this by specifying -s option with git commit command.
  2. When updating strings in the CLI tool, you need a commit to update the templates:
make i18n
git commit -a -s -m "i18n: Update translation templates" po/
  1. The current commit should look more like this: incus/cluster: Add customizable columns to list

Thanks!

@stgraber
Copy link
Member

I've done the following:

  • Re-title the PR
  • Cleanup the PR description
  • Moved the Signed-off-by to the commit message itself
  • Updated the commit author to match the signed-off-by string
  • Added a translation template (result of make i18n)
  • Ran gofmt -s -w ./ to fix some coding style issues
  • Ran make static-analysis and fixed the issues it detected

On top of those small easy fixes, I also noticed that you had put mention of filtering in the description but cluster list doesn't actually support filtering, so I've taken out that part of the description.

@stgraber
Copy link
Member

@presztak thanks for the review! I think you caught all the same stuff I did :)

@stgraber stgraber merged commit a921c39 into lxc:main Apr 24, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants