feat: add gomock/mockgen infrastructure for service mocks #10
Conversation
WalkthroughThe pull request introduces mockgen v0.6.0 as a development tool for automated mock generation. It adds mockgen to the .bingo/ tool management infrastructure with corresponding Makefile targets (generate-mocks and generate-all), updates service files with go:generate directives to auto-generate mocks, adds required dependencies to go.mod, migrates the cluster_nodepools_test.go test file from hand-written mocks to gomock-based mocks, and extends .gitignore to exclude generated mock files. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Makefile (1)
50-51:generate-mockstarget and help wiring are solid; consider documenting mockgen dependencyThe new help entry and
generate-mockstarget (${GO} generate ./pkg/services/...) cleanly hook into the existing tooling and align with thego:generatedirectives in the services package. To keep codegen reproducible across environments, consider adding a short note or helper target that installsmockgen(or otherwise documenting that it must be available on$PATH) somake generate-mocksworks out of the box.Also applies to: 222-225
pkg/handlers/cluster_nodepools_test.go (1)
12-186: Gomock-based handler tests are well-structured; only minor optional tidy-upsThe migration to gomock here looks solid: you create a per-subtest
gomock.Controller, deferFinish(), and usesetupMocksto define clear expectations for the success and error scenarios (cluster not found, nodepool not found, wrong owner), which nicely guards the handler’s behavior. As optional polish, you could 1) factor the repeatedNewMockClusterService/NewMockNodePoolService/NewMockGenericServiceconstruction into a small helper, and/or 2) switch from repeatedRegisterTestingT(t)calls togomega.NewWithT(t)inside the subtests to avoid relying on global Gomega state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
Makefile(2 hunks)go.mod(1 hunks)pkg/handlers/cluster_nodepools_test.go(8 hunks)pkg/services/adapter_status.go(1 hunks)pkg/services/adapter_status_mock.go(1 hunks)pkg/services/cluster.go(1 hunks)pkg/services/cluster_mock.go(1 hunks)pkg/services/generic.go(1 hunks)pkg/services/generic_mock.go(1 hunks)pkg/services/node_pool.go(1 hunks)pkg/services/node_pool_mock.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use structured logging via logger.NewOCMLogger(ctx) with context-aware fields including opid, accountID, and tx_id
Files:
pkg/services/generic.gopkg/services/generic_mock.gopkg/handlers/cluster_nodepools_test.gopkg/services/adapter_status.gopkg/services/cluster_mock.gopkg/services/node_pool.gopkg/services/node_pool_mock.gopkg/services/adapter_status_mock.gopkg/services/cluster.go
**/pkg/services/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Calculate aggregate status.phase from adapter conditions (Ready if all adapters=True, Failed if any=False, NotReady otherwise) without manual phase setting
Files:
pkg/services/generic.gopkg/services/generic_mock.gopkg/services/adapter_status.gopkg/services/cluster_mock.gopkg/services/node_pool.gopkg/services/node_pool_mock.gopkg/services/adapter_status_mock.gopkg/services/cluster.go
**/pkg/{services,dao}/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Files:
pkg/services/generic.gopkg/services/generic_mock.gopkg/services/adapter_status.gopkg/services/cluster_mock.gopkg/services/node_pool.gopkg/services/node_pool_mock.gopkg/services/adapter_status_mock.gopkg/services/cluster.go
**/pkg/handlers/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
HTTP handlers must follow the pattern: parse request → call service/DAO → handle errors with errors.SendError() → send response with appropriate status code
Files:
pkg/handlers/cluster_nodepools_test.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
All unit tests must run with OCM_ENV=unit_testing environment variable for proper test isolation
Files:
pkg/handlers/cluster_nodepools_test.go
{Makefile,*.mk,**/Makefile,**/*.mk}
📄 CodeRabbit inference engine (CLAUDE.md)
Build Go binary using FIPS-compliant crypto with CGO_ENABLED=1 and GOEXPERIMENT=boringcrypto for enterprise/government deployments
Files:
Makefile
{Makefile,*.mk,**/Makefile,**/*.mk,docker-compose*,**/Dockerfile}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Podman for containerized development and build consistency (database, code generation)
Files:
Makefile
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/{services,dao}/*.go : Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*model*,*dao*,*service*}.go : Use Kubernetes-style conditions in adapter status to support multiple independent adapters without coordination
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/{services,dao}/*.go : Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Applied to files:
pkg/services/generic.gopkg/services/generic_mock.gopkg/services/adapter_status.gopkg/services/cluster_mock.gopkg/services/node_pool.gopkg/services/adapter_status_mock.gopkg/services/cluster.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/api/*embed*.go : Embed OpenAPI specification at compile time using Go 1.16+ //go:embed directive for runtime availability and self-contained binary
Applied to files:
pkg/services/generic.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/test/factories/**/*.go : Use test factories in test/factories/ for consistent test data generation with builder pattern
Applied to files:
pkg/services/generic.gopkg/services/node_pool.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/handlers/*.go : HTTP handlers must follow the pattern: parse request → call service/DAO → handle errors with errors.SendError() → send response with appropriate status code
Applied to files:
pkg/handlers/cluster_nodepools_test.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*handler*,*error*}.go : Use structured error type from pkg/errors/ with HttpCode, Code, and Reason fields for consistent API error responses
Applied to files:
pkg/handlers/cluster_nodepools_test.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : NodePool resources must enforce owner_references.id (parent cluster ID) via foreign key constraint in database schema
Applied to files:
pkg/handlers/cluster_nodepools_test.gopkg/services/node_pool.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*model*,*dao*,*service*}.go : Use Kubernetes-style conditions in adapter status to support multiple independent adapters without coordination
Applied to files:
pkg/services/adapter_status.gopkg/services/adapter_status_mock.gopkg/services/cluster.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : Use polymorphic adapter_statuses table with owner_type and owner_id columns to avoid separate status tables for each resource type
Applied to files:
pkg/services/adapter_status.gopkg/services/adapter_status_mock.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/services/*.go : Calculate aggregate status.phase from adapter conditions (Ready if all adapters=True, Failed if any=False, NotReady otherwise) without manual phase setting
Applied to files:
pkg/services/adapter_status.gopkg/services/adapter_status_mock.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to openapi/openapi.yaml : Maintain openapi/openapi.yaml as the source of truth (32KB with $ref) and regenerate pkg/api/openapi/api/openapi.yaml (44KB fully resolved) via 'make generate' using Podman
Applied to files:
Makefile
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to pkg/api/openapi/api/openapi.yaml : Only edit openapi/openapi.yaml as the source; the generated pkg/api/openapi/api/openapi.yaml will be overwritten by 'make generate'
Applied to files:
Makefile
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Use TypeSpec to define API specifications, which compile to OpenAPI 3.0.3, then generate Go models using openapi-generator-cli v7.16.0
Applied to files:
Makefile
🧬 Code graph analysis (5)
pkg/services/generic_mock.go (2)
pkg/api/metadata_types.go (1)
PagingMeta(45-49)pkg/errors/errors.go (1)
ServiceError(94-101)
pkg/handlers/cluster_nodepools_test.go (7)
pkg/services/cluster_mock.go (2)
MockClusterService(22-26)NewMockClusterService(34-38)pkg/services/node_pool_mock.go (2)
MockNodePoolService(22-26)NewMockNodePoolService(34-38)pkg/services/generic_mock.go (2)
MockGenericService(22-26)NewMockGenericService(34-38)pkg/api/cluster_types.go (1)
Cluster(13-36)pkg/api/metadata_types.go (1)
Meta(37-42)pkg/api/node_pool_types.go (1)
NodePool(14-42)pkg/errors/errors.go (1)
NotFound(160-162)
pkg/services/cluster_mock.go (2)
pkg/api/cluster_types.go (2)
ClusterList(38-38)Cluster(13-36)pkg/errors/errors.go (1)
ServiceError(94-101)
pkg/services/node_pool_mock.go (3)
pkg/api/node_pool_types.go (2)
NodePoolList(44-44)NodePool(14-42)pkg/errors/errors.go (1)
ServiceError(94-101)test/helper.go (1)
Helper(54-67)
pkg/services/adapter_status_mock.go (3)
pkg/api/adapter_status_types.go (2)
AdapterStatusList(34-34)AdapterStatus(13-32)pkg/errors/errors.go (1)
ServiceError(94-101)test/helper.go (1)
Helper(54-67)
🔇 Additional comments (9)
go.mod (1)
115-123: New gomock/tooling dependencies align with mock-based testingThe added indirect requirements for gomock and Go tooling at lines 115–123 match the new mockgen/go:generate workflow and don’t affect runtime behavior; they look fine. Please just ensure
go mod tidyand your usual CI (lint/tests) still pass with these versions.pkg/services/adapter_status.go (1)
11-12: go:generate directive for AdapterStatusService is correctly wiredThe mockgen directive targets this interface file and emits
adapter_status_mock.goin the sameservicespackage; this is consistent with the rest of the services and has no runtime impact.pkg/services/node_pool.go (1)
15-16: NodePoolService mock generation hook looks correctThe
go:generateline correctly invokes mockgen onnode_pool.goand writesnode_pool_mock.gointo theservicespackage, matching the interface location and pattern used elsewhere.pkg/services/cluster.go (1)
15-16: ClusterService mock generation directive matches the interface fileThe
go:generatecomment correctly targetscluster.goand emitscluster_mock.goin theservicespackage, consistent with the other service mocks and with no effect on runtime behavior.pkg/services/generic_mock.go (1)
1-58: MockGenericService correctly mirrors GenericService.ListThe generated
MockGenericService(constructor, EXPECT recorder, andListmethod) matches theGenericServiceinterface signature and uses standard gomockCall/RecordCallWithMethodTypepatterns, so it should integrate cleanly with existing code and the newgo:generateworkflow.pkg/services/generic.go (1)
24-25: Added go:generate hook for GenericService mocks is appropriateThe mockgen directive ties
generic_mock.gogeneration directly togeneric.go, keeping the mock aligned with theGenericServiceinterface and matching the pattern used by the other services.pkg/services/cluster_mock.go (1)
1-175: LGTM! Generated mock follows standard gomock patterns.This is a properly generated gomock mock for ClusterService. The structure, method signatures, and recorder implementations all follow gomock conventions correctly. Remember to regenerate this file using
make generate-mockswhenever the ClusterService interface changes.pkg/services/node_pool_mock.go (1)
1-175: LGTM! Generated mock follows standard gomock patterns.This is a properly generated gomock mock for NodePoolService. The implementation is consistent with other service mocks in the PR and follows gomock conventions correctly. Remember to regenerate using
make generate-mockswhen the NodePoolService interface is updated.pkg/services/adapter_status_mock.go (1)
1-178: LGTM! Generated mock follows standard gomock patterns.This is a properly generated gomock mock for AdapterStatusService. The implementation correctly handles all method signatures including the paginated query with three return values. Remember to regenerate using
make generate-mockswhen the AdapterStatusService interface changes.
- Add go.uber.org/mock v0.6.0 dependency - Add go:generate directives to service interfaces: - pkg/services/cluster.go - pkg/services/node_pool.go - pkg/services/adapter_status.go - pkg/services/generic.go - Generate mock implementations for all service interfaces - Add 'make generate-mocks' target to Makefile - Update pkg/handlers/cluster_nodepools_test.go to use generated gomock mocks with EXPECT() pattern instead of hand-written mocks Closes: HYPERFLEET-290 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove *_mock.go files from version control - Add *_mock.go pattern to .gitignore - Add generate-all target to Makefile (runs generate + generate-mocks) - Make binary and install targets depend on generate-all Generated code is now created on-demand during build/test. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add go.uber.org/mock/mockgen v0.6.0 to bingo - Update generate-mocks target to depend on $(MOCKGEN) - Update go:generate directives to use mockgen-v0.6.0 Ensures all developers and CI use the same mockgen version. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
ciaranRoche
force-pushed
the
HYPERFLEET-290
branch
from
e56b58c to
a366fd6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/services/adapter_status.go (1)
11-11: Pinned mockgen directive forAdapterStatusServicelooks goodThe
//go:generateline is correctly scoped to this file, pins the mockgen version via the binary name, and writes toadapter_status_mock.goin the same package, which matches the new*_mock.goignore rule. Just ensure your documented workflows (make generate-mocks/generate-all) are the recommended way to rungo generateso the pinned binary is onPATH..bingo/README.md (1)
13-13: Clarify Go version vs FIPS runtime requirementThe broader “Go 1.24.x or 1.25.x” requirement is fine for tooling, but per project guidelines the runtime build should still be Go 1.24.9 with FIPS/boringcrypto enabled. Consider explicitly mentioning that this section is for developer tooling and pointing readers back to
CLAUDE.md(or the build docs) for the authoritative runtime version and FIPS flags.pkg/services/node_pool.go (1)
15-15: NodePoolService mockgen directive is consistent and well-scopedThe
go:generateline correctly targetsnode_pool.go, writesnode_pool_mock.gointo theservicespackage, and matches the*_mock.goignore pattern; it’s consistent with the other service interfaces. As with the other directives, make sure your standard dev/CI entrypoint (make generate-mocks) guaranteesmockgen-v0.6.0is onPATHwhen runninggo generate.pkg/services/cluster.go (1)
15-15: ClusterService mockgen directive is consistent and non-invasiveThe
go:generateline correctly pins mockgen, writescluster_mock.gointo theservicespackage, and doesn’t affect the existing adapter status aggregation logic, which remains compliant with the service guidelines.Makefile (1)
132-142: Consider extracting version check to reduce target length.The static analysis tool flags the
installtarget body as exceeding the recommended 5 lines. The version check warning block (lines 134-141) is duplicated fromverify.If desired, you could extract this to a shared target:
check-go-version: @ ${GO} version | grep -q "$(GO_VERSION)" || \ ( printf '\033[41m\033[97m\n'; \ echo "* Your go version is not the expected $(GO_VERSION) *" | sed 's/./*/g'; \ echo "* Your go version is not the expected $(GO_VERSION) *"; \ echo "* Your go version is not the expected $(GO_VERSION) *" | sed 's/./*/g'; \ printf '\033[0m'; )Then reuse it in both
verifyandinstall. This is optional and can be deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.bingo/mockgen.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
.bingo/README.md(1 hunks).bingo/Variables.mk(1 hunks).bingo/mockgen.mod(1 hunks).bingo/variables.env(1 hunks).gitignore(1 hunks)Makefile(3 hunks)go.mod(1 hunks)pkg/handlers/cluster_nodepools_test.go(8 hunks)pkg/services/adapter_status.go(1 hunks)pkg/services/cluster.go(1 hunks)pkg/services/generic.go(1 hunks)pkg/services/node_pool.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .bingo/mockgen.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/services/generic.go
🧰 Additional context used
📓 Path-based instructions (6)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Files:
pkg/handlers/cluster_nodepools_test.gopkg/services/adapter_status.gopkg/services/cluster.gopkg/services/node_pool.go
pkg/{handlers,services}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Always retrieve database session from context via db.NewContext() instead of creating new connections
Files:
pkg/handlers/cluster_nodepools_test.gopkg/services/adapter_status.gopkg/services/cluster.gopkg/services/node_pool.go
pkg/handlers/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Files:
pkg/handlers/cluster_nodepools_test.go
pkg/{handlers,services,dao}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/{handlers,services,dao}/**/*.go: Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Files:
pkg/handlers/cluster_nodepools_test.gopkg/services/adapter_status.gopkg/services/cluster.gopkg/services/node_pool.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement JWT authentication middleware that validates tokens and extracts account ID and username from claims
Files:
pkg/handlers/cluster_nodepools_test.gopkg/services/adapter_status.gopkg/services/cluster.gopkg/services/node_pool.go
pkg/services/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Files:
pkg/services/adapter_status.gopkg/services/cluster.gopkg/services/node_pool.go
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/api/openapi/**/*.go : Use make generate to regenerate Go models from openapi/openapi.yaml via openapi-generator-cli v7.16.0 in Podman
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/api/openapi/**/*.go : Use make generate to regenerate Go models from openapi/openapi.yaml via openapi-generator-cli v7.16.0 in Podman
Applied to files:
.bingo/variables.envpkg/services/adapter_status.go.bingo/Variables.mk.gitignorepkg/services/cluster.gopkg/services/node_pool.goMakefile
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to **/*.go : Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Applied to files:
.bingo/variables.env.bingo/README.mdMakefile
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/handlers/**/*.go : Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Applied to files:
pkg/handlers/cluster_nodepools_test.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/{handlers,services,dao}/**/*.go : Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Applied to files:
pkg/handlers/cluster_nodepools_test.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/{handlers,services,dao}/**/*.go : Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Applied to files:
pkg/handlers/cluster_nodepools_test.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/{handlers,services}/**/*.go : Always retrieve database session from context via db.NewContext() instead of creating new connections
Applied to files:
pkg/handlers/cluster_nodepools_test.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/db/migrations/**/*.go : Use polymorphic adapter_statuses table with owner_type and owner_id columns instead of separate status tables per resource type
Applied to files:
pkg/services/adapter_status.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/services/**/*.go : Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Applied to files:
pkg/services/adapter_status.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/dao/**/*.go : Increment generation field on each spec update to enable optimistic concurrency control
Applied to files:
pkg/services/adapter_status.gopkg/services/cluster.gopkg/services/node_pool.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/api/**/*.go : Use //go:embed directive to embed the OpenAPI specification at compile time from pkg/api/openapi/api/openapi.yaml
Applied to files:
pkg/services/adapter_status.go.gitignorepkg/services/cluster.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to test/integration/**/*.go : All API endpoints must be integrated tested in test/integration/ covering CRUD operations, pagination, search, and error cases
Applied to files:
.gitignore
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to test/factories/**/*.go : Use test data factories in test/factories/ to provide consistent test data instead of hardcoding values
Applied to files:
.gitignore
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Generate OpenAPI code by running make generate whenever the TypeSpec specification changes
Applied to files:
Makefile
🧬 Code graph analysis (1)
pkg/handlers/cluster_nodepools_test.go (4)
pkg/api/cluster_types.go (1)
Cluster(13-36)pkg/api/metadata_types.go (1)
Meta(37-42)pkg/api/node_pool_types.go (1)
NodePool(14-42)pkg/errors/errors.go (1)
NotFound(160-162)
🪛 checkmake (0.2.2)
Makefile
[warning] 132-132: Target body for "install" exceeds allowed length of 5 (9).
(maxbodylength)
🔇 Additional comments (10)
.gitignore (1)
53-55: Ignoring*_mock.gois appropriate for generated gomock filesThis matches the generated mock filenames and keeps the repo clean while still allowing tests to import them from the build workspace.
go.mod (1)
115-123: New mock/tooling dependencies are appropriate; ensure module state is tidyAdding
go.uber.org/mockplus thex/modandx/toolsversions fits the new gomock/mockgen workflow; nothing here looks problematic. Please just confirm that:
go mod tidyproduces a no-op diff, and- the versions used here are consistent with
.bingo/mockgen.modand your CI Go toolchain..bingo/Variables.mk (1)
38-42: MOCKGEN bingo wiring mirrors existing tooling correctlyThe
MOCKGENvariable and build rule follow the same pattern asGOTESTSUM, and the target pathgo.uber.org/mock/mockgenmatches the mockgen module layout. Just verify that.bingo/mockgen.modpins the same version (v0.6.0) and that CI invokes this target before any mock generation..bingo/variables.env (1)
15-15: MOCKGEN env var aligns with Variables.mkThe
MOCKGENenvironment variable matches the binary path defined in.bingo/Variables.mk, so sourcing this file gives a consistent CLI entrypoint for mock generation.pkg/handlers/cluster_nodepools_test.go (4)
12-12: LGTM on gomock import.Using
go.uber.org/mock/gomockis the correct modern import path for gomock (the uber fork that supersedes the archivedgithub.com/golang/mock).
39-65: Mock setup pattern is correct.The gomock setup follows best practices: creating mocks via
NewMock*Service(ctrl)and configuring expectations withEXPECT(). The use ofgomock.Any()for the context parameter is appropriate since the context itself is not under test.Note:
mockGenericSvcis created but has no expectations set. This is fine if theGethandler doesn't call GenericService methods—gomock will fail the test if any unexpected calls occur.
150-155: LGTM on controller lifecycle.Creating a new
gomock.Controllerper test iteration and deferringctrl.Finish()is the correct pattern for table-driven tests. This ensures proper mock verification and cleanup.
69-143: Good test coverage for error scenarios.The test cases properly cover the key error paths:
- Cluster not found (404)
- NodePool not found (404)
- NodePool ownership validation (different cluster ID returns 404)
Using
errors.NotFound()frompkg/errorsaligns with the structured ServiceError pattern per coding guidelines.Makefile (2)
126-132: Dependencies correctly updated.Both
binaryandinstallnow depend ongenerate-all, which includesgenerate-mocks. This addresses the concern from the previous review about ensuring mocks are generated before building/testing when generated code is not committed.The dependency chain
test → install → generate-all → generate-mocksensures tests always have fresh mocks.
224-231: LGTM on mock generation targets.The
generate-mockstarget correctly:
- Depends on
$(MOCKGEN)to ensure the tool is available- Uses
go generateto invoke mockgen via directives in source files- Is scoped to
./pkg/services/...The
generate-allaggregate target provides a convenient way to regenerate all code artifacts.
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ciaranRoche, rh-amarin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 8af50c7
into
openshift-hyperfleet:main
2 participants
Summary
Add gomock/mockgen infrastructure for generating service mocks, with version-pinned tooling via bingo.
Changes
make generate-mocks- Generate mock implementationsmake generate-all- Generate all code (openapi + mocks)binaryandinstalltargets depend ongenerate-allGenerated Code Policy
Generated mock files are not committed to the repository. They are:
*_mock.go)make binary,make test, ormake generate-mocksThis aligns with the project policy to not commit generated code.
Testing
Closes: HYPERFLEET-290
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.