Skip to content

Fix regression in github enterprise url handling #40 #41

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.

Sign up for GitHub

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

Open
luna-veil-8080 wants to merge 1 commit into
base: main
Choose a base branch
from
+41 −15
Open

Fix regression in github enterprise url handling #40 #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"io"
"net/http"
"net/url"
"strings"
"time"
)

Expand Down Expand Up @@ -101,6 +102,16 @@ func (c *githubClient) withEnterpriseURL(baseURL string) (*githubClient, error)
return nil, fmt.Errorf("failed to parse base URL: %w", err)
}

if !strings.HasSuffix(base.Path, "/") {
base.Path += "/"
}

if !strings.HasSuffix(base.Path, "/api/v3/") &&
!strings.HasPrefix(base.Host, "api.") &&
!strings.Contains(base.Host, ".api.") {
base.Path += "api/v3/"
}

c.baseURL = base

return c, nil
Expand Down
45 changes: 30 additions & 15 deletions github_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,33 +12,48 @@ import (

func Test_githubClient_withEnterpriseURL(t *testing.T) {
Copy link
Owner

@jferrl jferrl Feb 7, 2026

Choose a reason for hiding this comment

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

Could you add test cases for the remaining branches?

https://github.example.com/api/v3/ — already fully suffixed, should stay as-is
https://ghes.api.example.com — .api. in host, should skip /api/v3/ append
https://github.example.com/ — trailing slash already present, should not double it

tests := []struct {
name string
baseURL string
wantErr bool
name string
baseURL string
expectedBaseURL interface{}
Copy link
Owner

@jferrl jferrl Feb 7, 2026

Choose a reason for hiding this comment

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

Prefer keeping the original wantErr bool + a separate expectedURL string field instead of using interface{} to represent both. This is more idiomatic Go and gives type safety at compile time.

}{
{
name: "valid URL",
baseURL: "https://github.example.com",
wantErr: false,
name: "valid URL with subdomain",
baseURL: "https://api.github.example.com",
expectedBaseURL: "https://api.github.example.com/",
},
{
name: "valid URL with path",
baseURL: "https://github.example.com/api/v3",
expectedBaseURL: "https://github.example.com/api/v3/",
},
{
name: "valid URL without path",
baseURL: "https://github.example.com",
expectedBaseURL: "https://github.example.com/api/v3/",
},
{
name: "invalid URL with control characters",
baseURL: "ht\ntp://invalid",
wantErr: true,
name: "invalid URL with control characters",
baseURL: "ht\ntp://invalid",
expectedBaseURL: nil,
},
{
name: "URL with spaces",
baseURL: "http://invalid url with spaces",
wantErr: true,
name: "URL with spaces",
baseURL: "http://invalid url with spaces",
expectedBaseURL: nil,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
client := newGitHubClient(&http.Client{})
_, err := client.withEnterpriseURL(tt.baseURL)
if (err != nil) != tt.wantErr {
t.Errorf("withEnterpriseURL() error = %v, wantErr %v", err, tt.wantErr)
githubClient, err := client.withEnterpriseURL(tt.baseURL)

if err != nil {
Copy link
Owner

@jferrl jferrl Feb 7, 2026

Choose a reason for hiding this comment

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

This silently passes when an error occurs and expectedBaseURL is nil, but doesn't assert that an error was expected. The original if (err != nil) != tt.wantErr pattern is stricter — it catches both unexpected errors and missing expected errors.

if tt.expectedBaseURL != nil {
t.Errorf("withEnterpriseURL(%v) error = %v", tt.baseURL, err)
}
} else if githubClient.baseURL.String() != tt.expectedBaseURL {
t.Errorf("withEnterpriseURL(%v) expected = %v, received = %v", tt.baseURL, tt.expectedBaseURL, githubClient.baseURL)
}
})
}
Expand Down