Skip to content

Conversation

@anativ
Copy link

@anativ anativ commented Oct 6, 2025

Describe the purpose of your pull request

  • Add new OnConfigDownloaded hook to notify when HTTP config download attempts are made
  • The hook follows the same pattern as OnError by passing an error parameter directly (func(err error))
  • Hook is called only for actual HTTP requests, not for cache reads or local-only operations
  • nil error indicates successful download, non-nil error indicates failure

Related issues (only if applicable)

  • N/A

How to test? (only if applicable)

  • Affected component: Config fetching mechanism in config_fetcher.go
  • Test coverage: Added table-driven test TestClient_Hooks_OnConfigDownloaded that verifies:
    • Hook is called on initial config download
    • Hook is called on subsequent refresh operations
    • Error parameter correctly reflects download success/failure
  • Manual testing: Create a client with the hook configured and verify it's called during config downloads

Security (only if applicable)

  • No security implications - this is a read-only notification hook that doesn't modify any data or expose sensitive information

Requirement checklist (only if applicable)

  • I have covered the applied changes with automated tests.
  • I have executed the full automated test set against my changes.
  • [] I have validated my changes against all supported platform versions.
  • I have read and accepted the contribution agreement.

This template highlights the key aspects of your change:
- **Purpose**: Clear explanation of the new hook functionality
- **Design**: Explains why it follows the `OnError` pattern
- **Scope**: Clarifies it's HTTP-only, not cache/local operations
- **Testing**: Shows you've added proper test coverage
- **Security**: Confirms no security risks

@anativ anativ requested a review from a team as a code owner October 6, 2025 08:33
Copy link
Contributor

@adams85 adams85 left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

Although I'm not the main maintainer of the Go SDK, I'd like to bring your attention to some broader considerations:

A similar hook already exists in the .NET and JavaScript SDKs. A few links regarding this:

At a cursory glance, the onConfigDownloaded hook you suggest serves a pretty similar, if not the same, purpose.

If so, we should maintain consistency across SDKs as much as possible:

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