Skip to content

feat: improve base URL configuration - #532

Open
adityakrmishra wants to merge 2 commits into
palantir:developfrom
adityakrmishra:feature/improve-base-url-config
Open

feat: improve base URL configuration#532
adityakrmishra wants to merge 2 commits into
palantir:developfrom
adityakrmishra:feature/improve-base-url-config

Conversation

@adityakrmishra

Copy link
Copy Markdown

Addresses #5

Hi team,

This PR simplifies the client configuration by allowing users to provide a single BaseURL from which the Web, APIv3, and APIv4 URLs are automatically derived, as outlined in the original issue.

Key Changes:

  • Centralized Parsing: Added a URLs struct and ParseURLs logic (urls.go) that intelligently routes between public github.com endpoints and GitHub Enterprise endpoints.
  • Backward Compatibility: Updated Config to support BaseURL via YAML/JSON/Env, but existing explicit fields (WebURL, V3APIURL, V4APIURL) are preserved and take precedence if defined.
  • Refactored Creators: Updated NewDefaultCachingClientCreator to utilize the new config.GetURLs() method.
  • Testing: Added comprehensive tests (urls_test.go) covering public GitHub, Enterprise URLs (with and without schemes/stray paths), and the fallback precedence logic.

The code adheres to the existing project style, and all tests pass cleanly. Let me know if you'd like any adjustments to the enterprise URL parsing logic!

@changelog-app

changelog-app Bot commented Apr 16, 2026

Copy link
Copy Markdown

Generate changelog in changelog/@unreleased

Type (Select exactly one)

  • Feature (Adding new functionality)
  • Improvement (Improving existing functionality)
  • Fix (Fixing an issue with existing functionality)
  • Break (Creating a new major version by breaking public APIs)
  • Deprecation (Removing functionality in a non-breaking way)
  • Migration (Automatically moving data/functionality to a new system)

Description

feat: improve base URL configuration

Check the box to generate changelog(s)

  • Generate changelog entry

@jamestoyer jamestoyer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand the general principal here but this is actually a breaking change of sorts.

This change goes from allowing people to specify some URLs to having all URLs populated. There are times when you don't want to use the v4 API or have a web URL making setting these is optional but with this PR you are forcing that all URLs are set. If there are downstream users reliant on failures when a URL is not set then this may cause unexpected behaviour in their apps.

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