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

Modify to use loader and inventory client interfaces in cli-utils #3483

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

chunglu-chou
Copy link
Contributor

Remove the forked status command from cli-utils by implementing the loader and inventory client interfaces and utilize the command Runner code in cli-utils

Copy link
Contributor

@mortent mortent left a comment

Choose a reason for hiding this comment

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

Mostly looks good. Great to get rid of the fork.

@@ -45,13 +46,15 @@ func GetLiveCommand(ctx context.Context, _, version string) *cobra.Command {
}

f := newFactory(liveCmd, version)
invFactory := live.NewClusterClientFactory()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just keep this inside the cmdstatus package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morten Do you mean the NewClusterClientFactory function and the struct related to it or you are just saying this line could be placed inside the cmdstatus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the test utilizes the FakeClusterClient which is a different one, so the invFactory needs to be an input argument, otherwise the test will fail. But I think it's ok to move the implementation of the client factory into cmdstatus.

@@ -0,0 +1,61 @@
package live
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can keep this one too in the cmdstatus package. It might be that we want to reuse it in the other kpt live commands, but we can move it if that happens.

@mortent mortent merged commit c2f719e into kptdev:main Aug 25, 2022
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.

2 participants