Skip to content

client: centralize client initialization - #11085

Open
disksing wants to merge 4 commits into
masterfrom
client-centralize-initialization
Open

client: centralize client initialization#11085
disksing wants to merge 4 commits into
masterfrom
client-centralize-initialization

Conversation

@disksing

@disksing disksing commented Aug 3, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: ref #10989

The fixed-keyspace-ID and keyspace-name client constructors duplicate TLS, context, innerClient, and option initialization. Adding another initialization path on top of this duplication would make it easier for constructor behavior to drift.

What is changed and how does it work?

Extract the client state shared by the fixed-keyspace-ID and keyspace-name
initialization paths into one internal helper.

Keep service discovery initialization, the keyspace-name callback, logging,
and error handling in their existing paths.

This PR:

  • centralizes TLS, context, innerClient, and client option initialization;
  • keeps the fixed-keyspace-ID path using inner.init(nil);
  • keeps keyspace-name resolution and its initialization callback unchanged;
  • adds unit tests for the shared state, options, context, and TLS error path;
  • does not add API V3 types or change public APIs.

Check List

Tests

  • Unit test

Code changes

  • None

Side effects

  • None

Related changes

  • None

Release note

None.

Summary by CodeRabbit

  • Bug Fixes

    • Improved consistency across supported client connection methods.
    • Client settings, options, TLS configuration, and context handling are now applied consistently.
    • Invalid TLS configuration is rejected with an error instead of creating a client.

  • Tests

    • Added coverage for successful initialization, propagated settings, context handling, TLS validation, and invalid certificate material across supported client creation methods.

@disksing
client: centralize client initialization
c747042 
Signed-off-by: disksing <i@disksing.com>
@ti-chi-bot ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. labels Aug 3, 2026
@ti-chi-bot

ti-chi-bot Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign binshi-bing for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 3, 2026
@coderabbitai

coderabbitai Bot commented Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7380cf53-9293-4ed7-b9ad-1a47c9185d38

📥 Commits

Reviewing files that changed from the base of the PR and between a187877 and b559c9e.

📒 Files selected for processing (2)
  • client/client.go
  • client/client_test.go
📝 Walkthrough

Walkthrough

Client construction paths now share createClient. Callers perform initialization separately. Keyspace-name resolution remains in newClientWithKeyspaceName. Tests cover initialization state, context cancellation, and invalid TLS configuration.

Changes

Client construction

Layer / File(s) Summary
Shared client construction
client/client.go		 createClient centralizes shared client-state construction. Callers use it before initialization or keyspace-name resolution.
Client construction validation
client/client_test.go		 Tests cover initialization, propagated options, context cancellation, and invalid TLS handling across client constructors.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: bufferflies

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: centralized client initialization.
Description check ✅ Passed The description includes the required problem, issue number, implementation, checklist, related changes, and release note sections.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch client-centralize-initialization

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@disksing

disksing commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@codecov

codecov Bot commented Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.30%. Comparing base (a187877) to head (b559c9e).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #11085      +/-   ##
==========================================
+ Coverage   79.17%   79.30%   +0.12%     
==========================================
  Files         541      541              
  Lines       76487    76588     +101     
==========================================
+ Hits        60558    60736     +178     
+ Misses      11629    11574      -55     
+ Partials     4300     4278      -22
Flag Coverage Δ
unittests 79.30% <100.00%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@disksing
client: cover initialization error paths in tests
33c1dab 
Signed-off-by: disksing <i@disksing.com>
coderabbitai[bot]
coderabbitai Bot reviewed Aug 3, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@client/client_test.go`:
- Around line 124-138: The public-constructor tests around
TestNewClientWithKeyspaceInvalidTLSConfig and the following TLS case should pass
testClientURL instead of nil and assert the TLS-specific ErrCryptoX509KeyPair
with ErrorAs, while retaining the nil-client assertion, so the tests prove TLS
validation fails before keyspace or service-discovery errors.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: dd14908f-ac96-4763-bb8b-f75de6175243

📥 Commits

Reviewing files that changed from the base of the PR and between 72c1245 and 33c1dab.

📒 Files selected for processing (1)
  • client/client_test.go

Comment thread client/client_test.go
@disksing

disksing commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@disksing

disksing commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

addressed the review: pass testClientURL and assert the TLS-specific ErrCryptoX509KeyPair error with ErrorIs

@disksing

disksing commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

/test pull-unit-test-next-gen-3

4 similar comments
@disksing

disksing commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

/test pull-unit-test-next-gen-3

@disksing

disksing commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

/test pull-unit-test-next-gen-3

@disksing

disksing commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

/test pull-unit-test-next-gen-3

@disksing

disksing commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

/test pull-unit-test-next-gen-3

@disksing

disksing commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@disksing
disksing requested a review from Copilot August 3, 2026 06:16
Copilot AI reviewed Aug 3, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

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 refactors the PD client constructors by extracting the common “base client state” initialization (TLS config, derived context + cancel, innerClient fields, and option injection) into a single internal helper, reducing duplication between the fixed-keyspace-ID and keyspace-name initialization paths.

Changes:

  • Introduces an internal createClient(...) (*client, error) helper to build shared client state without performing service-discovery initialization.
  • Updates the fixed-keyspace-ID constructor path to reuse createClient and then call inner.init(nil) as before.
  • Adds unit tests to verify shared initialization state, option propagation, context cancellation propagation, and invalid TLS handling.

Reviewed changes

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

File Description
client/client.go Extracts shared client setup into createClient and updates constructors to reuse it while preserving existing init behavior.
client/client_test.go Adds tests covering shared initialization state, options/context propagation, and invalid TLS error behavior across constructors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

dco-signoff: yes Indicates the PR's author has signed the dco. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@disksing