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

Add format option to server ls command #383

Merged
merged 5 commits into from
Nov 26, 2024

Conversation

john801205
Copy link
Contributor

@john801205 john801205 commented Aug 8, 2024

Ref: rancher/rancher#48169

Summary

This PR allows users to specify format for the server ls command.

Usage

$ make build
$ ./bin/rancher server ls -h
List all servers

Usage:
  rancher server ls [OPTIONS]

Options:
   --format value  'json', 'yaml' or Custom format: '{{.Name}} {{.URL}}'
$ ./bin/rancher server ls --format '{{.Name}}'
beta
prod

@john801205 john801205 requested a review from a team as a code owner August 8, 2024 10:50
@crobby crobby self-requested a review September 26, 2024 20:09
@crobby crobby changed the base branch from v2.9 to main October 14, 2024 15:07
Copy link
Contributor

@crobby crobby left a comment

Choose a reason for hiding this comment

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

fairly straightforward change, but can you add a unit test for this as well?

@john801205
Copy link
Contributor Author

john801205 commented Nov 13, 2024

fairly straightforward change, but can you add a unit test for this as well?

@crobby
Do you want me to add some unit test for the returned configuration object cli.Command?
Otherwise, we already have some unit tests for the main logic here. We just don't allow the flag from the command line now.

cli/cmd/server_test.go

Lines 230 to 237 in f5d2eaa

{
name: "list servers with custom format",
format: "{{.URL}}",
expectedOutput: `https://myserver-1.com
https://myserver-2.com
https://myserver-3.com
`,
},

@crobby
Copy link
Contributor

crobby commented Nov 13, 2024

fairly straightforward change, but can you add a unit test for this as well?

@crobby Do you want me to add some unit test for the returned configuration object cli.Command? Otherwise, we already have some unit tests for the main logic here. We just don't allow the flag from the command line now.

cli/cmd/server_test.go

Lines 230 to 237 in f5d2eaa

{
name: "list servers with custom format",
format: "{{.URL}}",
expectedOutput: `https://myserver-1.com
https://myserver-2.com
https://myserver-3.com
`,
},

If you can add something like that, I think there is at least some value in it. I do agree that the "main logic" is indeed already tested here.

@john801205
Copy link
Contributor Author

john801205 commented Nov 17, 2024

If you can add something like that, I think there is at least some value in it. I do agree that the "main logic" is indeed already tested here.

@crobby Sure, just added. Also, I set the functions inside the object to nil to skip checking since it's not possible to check if they are equal at this moment.

crobby
crobby previously approved these changes Nov 25, 2024
@crobby crobby requested a review from a team November 25, 2024 15:33
@alegrey91 alegrey91 self-requested a review November 25, 2024 15:40
alegrey91
alegrey91 previously approved these changes Nov 25, 2024
)

func TestServerCommand(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I'm not sure why we need such a test. This is brittle and doesn't really test any functionality. I'd rather not have it at all.
/cc @crobby

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point. Maybe it would be fine to just remove the test and merge as it was.

Copy link
Contributor

Choose a reason for hiding this comment

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

@john801205 Sorry for the back-and-forth. Can you please revert the unit test changes and we'll merge the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, no problem. I have reverted the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

This reverts commit 5218715.
@pmatseykanets pmatseykanets merged commit 2cf4939 into rancher:main Nov 26, 2024
1 check passed
@pmatseykanets pmatseykanets changed the title enable server ls command to accept the format option Add format option to server ls command Nov 26, 2024
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.

5 participants