Skip to content

Conversation

@dbpolito
Copy link
Member

No description provided.

@dbpolito dbpolito requested review from Copilot and fabriciojs May 23, 2025 20:09
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for passing a Kubernetes cluster identifier through the Authenticate method to both deploy exec and logs commands.

  • Extended Authenticate signature to include a cluster parameter.
  • Updated the default implementation to set the cluster field in the request body.
  • Modified commands and tests to retrieve KOOL_CLOUD_CLUSTER from the environment and pass it into Authenticate.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
services/cloud/k8s/kubectl.go Extended Authenticate interface and implementation
services/cloud/k8s/kubectl_test.go Updated tests to call new Authenticate signature
commands/cloud_deploy_logs.go Fetch KOOL_CLOUD_CLUSTER env var and pass to Authenticate
commands/cloud_deploy_logs_test.go Updated fakeK8S to record new cluster argument
commands/cloud_deploy_exec.go Same env var handling for exec command
Comments suppressed due to low confidence (2)

services/cloud/k8s/kubectl_test.go:19

  • Consider adding a test case that supplies a non-empty cluster value and asserts that the underlying deployExec.Body receives the correct cluster field, ensuring the new parameter is forwarded properly.
if _, err := k.Authenticate("foo", "bar", ""); !errors.Is(err, expectedErr) {

services/cloud/k8s/kubectl.go:33

  • [nitpick] The Authenticate method signature has changed; please update any relevant doc comments or interface documentation to explain the new cluster parameter.
func (k *DefaultK8S) Authenticate(domain, service, cluster string) (cloudService string, err error) {

Comment on lines +76 to +78
cluster := e.env.Get("KOOL_CLOUD_CLUSTER")

if cloudService, err = e.cloud.Authenticate(domain, service, cluster); err != nil {
Copy link

Copilot AI May 23, 2025

Choose a reason for hiding this comment

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

[nitpick] The cluster fetch and authenticate call are duplicated in both logs and exec commands; consider extracting this logic into a shared helper method to reduce duplication.

Suggested change
cluster := e.env.Get("KOOL_CLOUD_CLUSTER")
if cloudService, err = e.cloud.Authenticate(domain, service, cluster); err != nil {
if cloudService, err = e.fetchAndAuthenticateCluster(domain, service); err != nil {

Copilot uses AI. Check for mistakes.
@dbpolito dbpolito merged commit bc4dfab into main May 23, 2025
8 checks passed
@dbpolito dbpolito deleted the kool_cloud_exec_log_cluster branch May 23, 2025 20:18
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.

3 participants