Skip to content

Remove eager Setup() call from ClientWithConfig.UnmarshalYAML#304

Merged
kke merged 6 commits intomainfrom
eager-setup
Mar 26, 2026
Merged

Remove eager Setup() call from ClientWithConfig.UnmarshalYAML#304
kke merged 6 commits intomainfrom
eager-setup

Conversation

@kke
Copy link
Copy Markdown
Contributor

@kke kke commented Mar 26, 2026

Setup() was called with no options during YAML unmarshaling, which caused it to no-op when Connect(ctx, opts...) was later called with options such as a logger or retry policy. Deferring setup to Connect (or an explicit Setup call) ensures those options are applied.

Setup() was called with no options during YAML unmarshaling, which caused
it to no-op when Connect(ctx, opts...) was later called with options such
as a logger or retry policy. Deferring setup to Connect (or an explicit
Setup call) ensures those options are applied.

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
@kke kke requested a review from Copilot March 26, 2026 12:34
@kke kke marked this pull request as ready for review March 26, 2026 12:34
Copy link
Copy Markdown

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 adjusts YAML unmarshaling for ClientWithConfig so it no longer eagerly initializes the underlying Client, ensuring Connect(ctx, opts...) options (e.g., logger/retry) are actually applied when the client is set up.

Changes:

  • Remove the eager Setup() call from ClientWithConfig.UnmarshalYAML.
  • Update the UnmarshalYAML doc comment to describe the new “defer setup to Connect/Setup” behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
Copy link
Copy Markdown

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 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
Copy link
Copy Markdown

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 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
Copy link
Copy Markdown

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 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
Copy link
Copy Markdown

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 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
Copy link
Copy Markdown

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 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@kke kke merged commit e384af6 into main Mar 26, 2026
15 checks passed
@kke kke deleted the eager-setup branch March 26, 2026 13:54
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