Skip to content

fix: Add option to list from remote cluster #1433

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dmartinol
Copy link

Initial commit for #1429

  • Adds runtime flag to thv list command to limit the extent of changes
  • Keeps fallback for in-cluster deployment

Keeping in draft mode until there's some decision about the other commands.

…lient configuration

- Introduced a new `--runtime` flag for the `thv list` command to specify the container runtime (docker, kubernetes).
- Updated the command's documentation to reflect the new flag.
- Enhanced the Kubernetes client to fall back to remote configuration if in-cluster configuration fails, improving error handling and logging.
- Added tests to ensure proper fallback behavior for Kubernetes client configuration.

Signed-off-by: [Daniele Martinoli] <[email protected]>
Signed-off-by: Daniele Martinoli <[email protected]>
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

I only have some very minor comments, but overall looks good. I haven't tried running the code though.

if err != nil {
return nil, fmt.Errorf("failed to get user home directory: %w", err)
}
kubeconfigPath = homeDir + "/.kube/config"
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, this will not work on Windows will it? I was browsing through the clientcmd package because I was not familiar with it and noticed that it exports RecommendedHomeFile, could we use that?

@@ -401,6 +480,7 @@ func (c *Client) ListWorkloads(ctx context.Context) ([]runtime.ContainerInfo, er

// List pods with the toolhive label
namespace := getCurrentNamespace()
fmt.Printf("listing pods in namespace %s", namespace)
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 this was left here by accident?

var err error

// First, try to create an in-cluster config
config, err = rest.InClusterConfig()
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to only do the fallback on rest.ErrNotInCluster?

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