refactor(unifi): decouple UniFi API implementation with interfaces #157
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The previous implementation concatenated -t and tag as a single string, which caused docker buildx imagetools to fail with "invalid reference format". Now using jq to output -t and tag on separate lines, creating properly structured bash arrays for the docker command. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add comprehensive build summaries to release and PR workflows showing: - Published image tags - Supported platforms (amd64, arm64) - Manifest digest - Pull commands - Cosign signature verification instructions (release only) This provides better visibility into build results directly in the GitHub Actions UI. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add `name` parameter to all workflow jobs for cleaner GitHub Actions UI: - "Build Image" / "Build and Push" / "Build and Test" - "Merge Manifests" - "Cleanup PR Images" Matrix jobs (amd64/arm64) now share the same display name, grouping them visually in the GitHub Actions interface instead of showing separate entries for each platform. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…e deployments Removing the GitHub environment from the matrix job eliminates duplicate deployment events in PR timeline (one per platform). The packages:write permission is sufficient for pushing to GHCR with pull_request_target, as the workflow runs with the target repository's token and uses trusted code from the default branch. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Empty commit to trigger workflow runs with updated changes. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add automatic cleanup of PR container images when PR is closed: - New cleanup-pr-image job triggered on pull_request_target closed event - Automatically finds and deletes the pr-<number> tagged image - Removes explicit GH_TOKEN env vars (gh CLI uses GITHUB_TOKEN automatically) This eliminates the need for manual cleanup workflow runs and ensures PR images don't accumulate in the registry. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
gh CLI in GitHub Actions requires explicit GH_TOKEN environment variable to be set. Without it, gh commands fail with exit code 4. Restoring the env block that was incorrectly removed. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Remove the manual cleanup workflow since PR images are now automatically deleted when the PR is closed via the cleanup-pr-image job in pr-build.yaml. The manual workflow is no longer needed and was redundant. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Introduce comprehensive interface layer to decouple UniFi API implementation from its usage, enabling dependency injection and improving testability. Architecture changes: - Split monolithic httpClient into layered components: * HTTPTransport: HTTP communication, auth, CSRF token management * UnifiAPIClient: DNS operations (GetEndpoints, CreateEndpoint, DeleteEndpoint) * RecordTransformer: DNS record format conversions (SRV, CNAME) * MetricsAdapter/LoggerAdapter: Abstraction over singleton dependencies New files: - interface.go: Core interface definitions (HTTPTransport, UnifiAPI, RecordTransformer, MetricsRecorder, Logger) - transport.go: HTTP layer implementation with authentication - transformer.go: DNS record transformation logic - adapters.go: Adapters for metrics and logger singletons Modified files: - client.go: Refactored to use injected dependencies - provider.go: Updated with dependency injection pattern - dnsprovider.go: Wires all dependencies together - types.go: Added ClientURLs type Benefits: - Clean separation of concerns (HTTP, API, business logic) - Easy mocking for unit tests - No direct singleton dependencies in core logic - Flexible implementation replacement - Better testability and maintainability Breaking changes: None - backward compatibility maintained through deprecated functions Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Update unit tests to work with the new interface-based architecture: - Replace direct httpClient usage with mock HTTPTransport - Add createTestClient helper for test setup - Add mockHTTPTransport implementation with proper error handling - Update TestSetHeaders to use mockHTTPTransport - All tests passing (0.614s) Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add extensive test coverage for the new interface-based architecture: - provider_test.go: mock-based tests for UnifiProvider using mocked API - transformer_test.go: DNS record transformation tests (SRV, A, CNAME, etc) - adapters_test.go: adapter delegation tests for metrics and logging Tests validate core functionality in isolation using the new interfaces, enabling easier testing and better maintainability. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Remove client_test.go which used httptest.NewServer() and caused port
exhaustion issues on macOS ("can't assign requested address").
The new interface-based tests (provider_test.go, transformer_test.go,
adapters_test.go) provide better unit test coverage using proper mocks
instead of real HTTP connections. These tests are faster, more reliable,
and don't depend on OS-level resources like ephemeral ports.
Coverage: 33.2% (focused on business logic through mocks vs HTTP plumbing)
Co-Authored-By: Claude <[email protected]>
Signed-off-by: Aleksei Sviridkin <[email protected]>
Signed-off-by: Aleksei Sviridkin <[email protected]>
Add tests for NewUnifiProvider and NewUnifiProviderFromConfig constructors to improve test coverage. Also add mock-based tests for client and transport layers to avoid port exhaustion issues on macOS. Coverage improved from 68.4% to 70.6%, with provider.go reaching 81%+ coverage on all functions. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add comprehensive tests for transport layer authentication flows: - Login function coverage with User/Password authentication - CSRF token handling and refresh - 401 Unauthorized retry logic with automatic re-login - Additional DoRequest error scenarios (context cancel, invalid URL) - Error response handling with and without messages Coverage improved from 50.8% to 84.4% for transport.go. Overall coverage reached 82.9% with all files above 80%. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Signed-off-by: Aleksei Sviridkin <[email protected]>
The ireturn linter issues in mock functions were already resolved by updating mock implementations to return concrete types where appropriate. The remaining //nolint:ireturn directives for IgnoredCNAMETargetsTotal() and SRVParsingErrorsTotal() were no longer needed and triggered nolintlint warnings. Changes: - Remove unused //nolint:ireturn directives from provider_test.go This ensures clean linter output with zero issues. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
|
✅ Container image built and pushed successfully! Image: Platforms: linux/amd64, linux/arm64 Pull command: docker pull ghcr.io/kashalls/external-dns-unifi-webhook:pr-157 |
Owner
|
@lexfrei Any chance you could look into this? Compared to when I was running on main before we made the changes (left-most side) and the dark green before this pr I was using PR-152's image. It looks like there is some heavy memory usage compared to the other images. 
|
Owner
{"time":"2025-11-24T15:21:14.990305831Z","level":"ERROR","source":{"function":"github.com/kashalls/external-dns-unifi-webhook/pkg/webhook.(*Webhook).Records","file":"/build/pkg/webhook/webhook.go","line":50},"msg":"error getting records","req_method":"GET","req_path":"/records","error":"failed to fetch DNS records: failed to fetch DNS records from UniFi: data error during unmarshal of API error response: unexpected end of JSON input"} |
Owner
|
@lexfrei Any updates on this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Refactor UniFi API implementation to use interface-based architecture for better testability and maintainability. This enables dependency injection, easier mocking, and cleaner separation of concerns between HTTP transport, API operations, and business logic.
Changes
HTTPTransport,UnifiAPI,RecordTransformer,MetricsRecorder,Logger) ininternal/unifi/interface.gotransport.go: HTTP layer with authentication and CSRF handlingclient.go: UniFi API operations (GetEndpoints, CreateEndpoint, DeleteEndpoint)transformer.go: DNS record transformations (SRV, A, CNAME, etc.)adapters.go: Adapters for metrics and logging to break singleton dependenciesType of Change
Testing
Tests now use proper mocks instead of real HTTP servers, making them faster and more reliable.
Checklist
Related Issues