Skip to content

Conversation

@conradj3
Copy link

@conradj3 conradj3 commented Oct 4, 2025

Problem

When running octopus login --ignore-ssl-errors after an initial successful login, the CLI crashes with a panic:

panic: interface conversion: http.RoundTripper is *apiclient.SpinnerRoundTripper, not *http.Transport

This occurs because:

  1. First login: HTTP client uses direct *http.Transport
  2. Subsequent logins: HTTP client uses *SpinnerRoundTripper wrapping *http.Transport (for interactive spinner display)
  3. The crash: Code assumed transport would always be *http.Transport and performed unsafe type assertion

Solution

Replaced the unsafe type assertion with proper type switching to handle multiple transport scenarios:

  • ✅ Direct *http.Transport (first login)
  • *SpinnerRoundTripper wrapping *http.Transport (subsequent logins)
  • ✅ Fallback for any other transport types

Changes

  • Modified SSL ignore logic in pkg/cmd/login/login.go to use type switching
  • Added comprehensive test coverage in pkg/cmd/login/login_ssl_test.go
  • All existing tests continue to pass

Testing

  • Unit tests for all transport scenarios
  • All existing login tests pass
  • CLI builds and runs successfully

Related Issues

Fixes the panic described in the original issue where users couldn't run octopus login --ignore-ssl-errors multiple times.

Copilot AI review requested due to automatic review settings October 4, 2025 23:58
@CLAassistant
Copy link

CLAassistant commented Oct 4, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

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 fixes a panic that occurs when running octopus login --ignore-ssl-errors on subsequent runs by replacing an unsafe type assertion with proper type switching to handle multiple HTTP transport scenarios.

  • Replaced unsafe type assertion with type switching to handle both direct *http.Transport and *SpinnerRoundTripper wrapping scenarios
  • Added comprehensive test coverage for all transport scenarios including edge cases
  • Maintained backward compatibility while preventing the panic on subsequent login attempts

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
pkg/cmd/login/login.go Replaced unsafe type assertion with type switching to handle multiple transport types safely
pkg/cmd/login/login_ssl_test.go Added comprehensive test coverage for SSL ignore logic with various transport scenarios

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

}
}

// applySSLIgnoreLogic contains the same logic as in loginRun for handling SSL
Copy link

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

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

The comment should clarify that this function mirrors the SSL ignore logic from the main implementation to ensure test accuracy.

Suggested change
// applySSLIgnoreLogic contains the same logic as in loginRun for handling SSL
// applySSLIgnoreLogic mirrors the SSL ignore logic from the main implementation (loginRun)
// to ensure test accuracy. If the SSL ignore logic in loginRun changes, this function
// should be updated accordingly to keep the test valid.

Copilot uses AI. Check for mistakes.
Copy link
Author

Choose a reason for hiding this comment

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

Comment Updated

Comment on lines 78 to 83
} else {
// If Next is not an http.Transport, replace it with one that has SSL verification disabled
transport.Next = &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
}
}
Copy link

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

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

This logic is duplicated between the test file and the main implementation. Consider extracting this SSL ignore logic into a shared utility function to reduce code duplication and ensure consistency.

Copilot uses AI. Check for mistakes.
Copy link
Author

Choose a reason for hiding this comment

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

Pushed recommended to remove duplicate logic, and extracted SSL ignore logic to a shared functionality.

@conradj3 conradj3 requested a review from Copilot October 5, 2025 00:17
Copy link
Contributor

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 22 to 26
transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
case *SpinnerRoundTripper:
// If the SpinnerRoundTripper's Next is an http.Transport, configure it
if httpTransport, ok := transport.Next.(*http.Transport); ok {
httpTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
Copy link

Copilot AI Oct 5, 2025

Choose a reason for hiding this comment

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

Same as above - consider preserving existing TLS configuration when possible. If httpTransport.TLSClientConfig already exists, you should modify only the InsecureSkipVerify field rather than replacing the entire config.

Suggested change
transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
case *SpinnerRoundTripper:
// If the SpinnerRoundTripper's Next is an http.Transport, configure it
if httpTransport, ok := transport.Next.(*http.Transport); ok {
httpTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
if transport.TLSClientConfig != nil {
transport.TLSClientConfig.InsecureSkipVerify = true
} else {
transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
}
case *SpinnerRoundTripper:
// If the SpinnerRoundTripper's Next is an http.Transport, configure it
if httpTransport, ok := transport.Next.(*http.Transport); ok {
if httpTransport.TLSClientConfig != nil {
httpTransport.TLSClientConfig.InsecureSkipVerify = true
} else {
httpTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
}

Copilot uses AI. Check for mistakes.
Copy link
Author

Choose a reason for hiding this comment

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

Modified and tested

conradj3 and others added 2 commits October 4, 2025 20:20
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@conradj3 conradj3 requested a review from Copilot October 5, 2025 00:25
Copy link
Contributor

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

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