move messaging models from pkg/compute to pkg/models/messages #4680
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request focus on integrating a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (18)
pkg/models/messages/logging.go (3)
3-8: Add field documentation and consider input validation.The struct would benefit from the following improvements:
- Add documentation comments explaining the purpose of each field, especially the difference between
TailandFollowflags- Consider adding validation for
ExecutionIDto ensure it's not emptyExample documentation:
type ExecutionLogsRequest struct { + // RoutingMetadata contains routing information for the request RoutingMetadata + // ExecutionID uniquely identifies the execution whose logs are being requested ExecutionID string + // Tail when true returns only the most recent log entries Tail bool + // Follow when true streams logs as they are generated Follow bool }
10-13: Clarify Address field type and add documentation.The struct would benefit from the following improvements:
- Add documentation comments explaining the purpose of each field
- Consider using a more specific type for
Address(e.g.,url.URLif it's a URL)- Consider adding validation to ensure
Addressis not emptyExample documentation:
+// ExecutionLogsResponse contains the location of execution logs and their status type ExecutionLogsResponse struct { + // Address specifies the location where logs can be accessed (TODO: specify format: URL/file path?) Address string + // ExecutionFinished indicates whether the execution has completed ExecutionFinished bool }
1-13: Well-structured message types for execution logging.The separation of concerns between request and response types is clean, and the move to pkg/models/messages improves code organization. The types provide a good foundation for handling execution logs across different components of the system.
Consider adding:
- A version field to support future API evolution
- An errors package for standardized error responses
pkg/models/messages/metadata.go (2)
5-8: Add field documentation and consider input validation.While the struct is well-structured, consider:
- Adding GoDoc comments for the struct and its fields to improve documentation
- Adding validation for peer ID format in methods that create this struct
Example documentation:
+// RoutingMetadata contains routing information for peer-to-peer communication type RoutingMetadata struct { + // SourcePeerID is the identifier of the sending peer SourcePeerID string + // TargetPeerID is the identifier of the receiving peer TargetPeerID string }
10-13: Add struct and field documentation.Consider adding GoDoc comments to improve code documentation.
Example documentation:
+// ExecutionMetadata contains identifiers for tracking execution instances type ExecutionMetadata struct { + // ExecutionID uniquely identifies an execution instance ExecutionID string + // JobID identifies the job associated with this execution JobID string }pkg/models/messages/execution.go (1)
7-15: Add field documentation for better maintainability.The cancel-related structures would benefit from documentation explaining the purpose of each field, especially:
- The relationship between
RoutingMetadataandExecutionMetadata- The expected format or constraints for
JustificationAdd documentation like this:
+// CancelExecutionRequest represents a request to cancel an ongoing execution type CancelExecutionRequest struct { RoutingMetadata + // ExecutionID uniquely identifies the execution to cancel ExecutionID string + // Justification provides the reason for cancellation Justification string } +// CancelExecutionResponse represents the response to a cancellation request type CancelExecutionResponse struct { ExecutionMetadata }pkg/models/messages/node.go (2)
5-13: Please clarify sequence number behavior on node rebootThe documentation mentions "reboots aside" when discussing the monotonically increasing sequence number. Consider documenting:
- How sequence numbers are initialized after a reboot
- How the server should handle sequence number resets
The type structure and documentation are otherwise well-designed, particularly the decision to avoid client-side timestamps.
Line range hint
14-40: Consider adding documentation to existing typesWhile the new
Heartbeattype is well-documented, the existing types lack similar documentation. Consider adding documentation to explain:
- The purpose of each request/response pair
- Any important behavior or constraints
- The relationship between related types (e.g., RegisterRequest/Response)
pkg/models/messages/bidding.go (4)
19-29: Consider adding field documentation.The structure is clean, but adding documentation for the fields would improve code maintainability.
type BidAcceptedRequest struct { RoutingMetadata + // ExecutionID uniquely identifies the execution this bid acceptance is for ExecutionID string + // Accepted indicates whether the bid was accepted Accepted bool + // Justification provides the reason for the acceptance decision Justification string }
30-39: Consider adding field documentation.The structure is clean, but adding documentation for the fields would improve code maintainability.
type BidRejectedRequest struct { RoutingMetadata + // ExecutionID uniquely identifies the execution this bid rejection is for ExecutionID string + // Justification provides the reason for the rejection Justification string }
40-48: Consider adding field documentation.While the type has good documentation, adding field-level documentation would improve clarity.
// BidResult is the result of the compute node bidding on a job that is returned // to the caller through a Callback. type BidResult struct { RoutingMetadata ExecutionMetadata + // Accepted indicates whether the bid was accepted Accepted bool + // Wait indicates if the compute node should wait before proceeding Wait bool + // Event contains the event details related to this bid result Event models.Event }
1-48: Well-structured message types package.The move of these types to a dedicated
messagespackage is a good architectural decision that:
- Centralizes message definitions
- Provides clear separation of concerns
- Makes the message contract explicit
- Follows Go package organization best practices
The consistent structure across all message types will make the codebase more maintainable.
pkg/test/compute/mocks.go (1)
30-35: Consider improving method signature formatting.While the type migration is correct, the method signature could be formatted more idiomatically.
-func (m ManagementEndpointMock) UpdateResources( - ctx context.Context, request messages.UpdateResourcesRequest) (*messages.UpdateResourcesResponse, error) { +func (m ManagementEndpointMock) UpdateResources(ctx context.Context, request messages.UpdateResourcesRequest) (*messages.UpdateResourcesResponse, error) {pkg/nats/proxy/management_proxy.go (1)
Line range hint
101-127: LGTM with a minor suggestion about timeout configurationThe generic implementation is clean and type-safe. Consider making the
requestTimeoutconfigurable through theManagementProxyParamsstruct for better flexibility in different environments.Consider this change:
type ManagementProxyParams struct { Conn *nats.Conn + RequestTimeout time.Duration } type ManagementProxy struct { conn *nats.Conn + requestTimeout time.Duration } func NewManagementProxy(params ManagementProxyParams) *ManagementProxy { + timeout := params.RequestTimeout + if timeout == 0 { + timeout = 2 * time.Second + } return &ManagementProxy{ conn: params.Conn, + requestTimeout: timeout, } }pkg/test/compute/cancel_test.go (1)
72-72: Consider replacing sleep with container readiness checkWhile the request type changes look good, the arbitrary 1-second sleep to wait for container activation could be flaky. Consider using Docker API's container inspection or health checks to ensure container readiness.
Example approach:
// Replace time.Sleep with container readiness check containerID := // get container ID from execution context for retries := 0; retries < maxRetries; retries++ { container, err := dockerClient.ContainerInspect(ctx, containerID) if err == nil && container.State.Running { break } time.Sleep(100 * time.Millisecond) }Also applies to: 81-83
pkg/compute/endpoint.go (1)
156-157: Consider using %v for state type formattingWhen formatting the error message, using
%vinstead of%sfor the state type might be more appropriate as it's an enum/custom type.- return messages.CancelExecutionResponse{}, fmt.Errorf("cannot cancel execution %s in state %s", + return messages.CancelExecutionResponse{}, fmt.Errorf("cannot cancel execution %s in state %v",pkg/orchestrator/callback.go (1)
168-171: Consider implementing proper cancellation handling.While the type change from
compute.CancelResulttomessages.CancelResultis correct, this appears to be a stub implementation. Consider enhancing it to:
- Update execution state
- Handle cleanup tasks
- Maintain execution history
Would you like help implementing a more comprehensive cancellation handler?
pkg/node/manager/node_manager.go (1)
Line range hint
138-138: Consider tracking the TODO comment about revision-aware Put endpointThis TODO suggests a potential improvement for concurrent updates handling.
Would you like me to create a GitHub issue to track the implementation of a revision-aware Put endpoint?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (37)
pkg/compute/bidder.go(6 hunks)pkg/compute/bidder_test.go(4 hunks)pkg/compute/callback_chain.go(2 hunks)pkg/compute/callback_mock.go(1 hunks)pkg/compute/endpoint.go(7 hunks)pkg/compute/executor.go(4 hunks)pkg/compute/executor_buffer.go(2 hunks)pkg/compute/management_client.go(4 hunks)pkg/compute/mocks.go(13 hunks)pkg/compute/types.go(2 hunks)pkg/models/messages/bidding.go(1 hunks)pkg/models/messages/execution.go(1 hunks)pkg/models/messages/logging.go(1 hunks)pkg/models/messages/metadata.go(1 hunks)pkg/models/messages/node.go(1 hunks)pkg/nats/proxy/callback_proxy.go(2 hunks)pkg/nats/proxy/compute_proxy.go(2 hunks)pkg/nats/proxy/management_handler.go(4 hunks)pkg/nats/proxy/management_proxy.go(4 hunks)pkg/node/heartbeat/client.go(2 hunks)pkg/node/heartbeat/heartbeat_test.go(2 hunks)pkg/node/heartbeat/server.go(4 hunks)pkg/node/heartbeat/types.go(0 hunks)pkg/node/manager/node_manager.go(6 hunks)pkg/node/ncl.go(1 hunks)pkg/orchestrator/callback.go(4 hunks)pkg/orchestrator/endpoint.go(2 hunks)pkg/orchestrator/planner/compute_forwarder.go(5 hunks)pkg/orchestrator/planner/compute_forwarder_test.go(2 hunks)pkg/orchestrator/planner/utils_test.go(3 hunks)pkg/test/compute/ask_for_bid_pre_approved_test.go(2 hunks)pkg/test/compute/ask_for_bid_test.go(3 hunks)pkg/test/compute/bid_accepted_test.go(3 hunks)pkg/test/compute/bid_rejected_test.go(3 hunks)pkg/test/compute/cancel_test.go(6 hunks)pkg/test/compute/mocks.go(1 hunks)pkg/test/compute/setup_test.go(7 hunks)
💤 Files with no reviewable changes (1)
- pkg/node/heartbeat/types.go
🔇 Additional comments (126)
pkg/models/messages/logging.go (1)
1-1: LGTM! Package name aligns with the new location.
The package name follows Go conventions and correctly reflects its location in the codebase.
pkg/models/messages/metadata.go (2)
1-3: LGTM! Clean package structure and imports.
Package name follows Go conventions and imports are minimal.
1-20: Verify complete migration of message types.
Let's verify that all references to these metadata types have been updated to use the new package.
✅ Verification successful
Let me verify if there are any usages of these types that haven't been migrated by checking for their usage patterns.
Let me fix the regex and try one more verification to ensure all usages are properly migrated:
Based on the verification results, I can now provide the final response:
Migration of metadata types appears complete and consistent
The verification shows that:
- No old definitions of these types exist in pkg/compute/
- All imports are using the new package path
pkg/models/messages - All usages of
RoutingMetadataandExecutionMetadataare properly using the new package - Field accesses are consistent with the new structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to these types in the old location
# and verify all imports of the new package
# Check for any remaining references to these types in pkg/compute
echo "Checking for remaining references in old location..."
rg "type (Routing|Execution)Metadata struct" pkg/compute/
# Verify all imports of the new package
echo "Checking usage of new package..."
rg "github.com/bacalhau-project/bacalhau/pkg/models/messages"
Length of output: 3169
Script:
#!/bin/bash
# Check for any remaining usages of these types that might not be properly migrated
echo "Checking for RoutingMetadata usage patterns..."
rg "RoutingMetadata\s*{" --type go
echo -e "\nChecking for ExecutionMetadata usage patterns..."
rg "ExecutionMetadata\s*{" --type go
echo -e "\nChecking for NewExecutionMetadata usage patterns..."
rg "NewExecutionMetadata\(" --type go
Length of output: 1469
Script:
#!/bin/bash
# Check for any remaining usages with fixed regex
echo "Checking for RoutingMetadata usage patterns..."
rg "RoutingMetadata *{" --type go
echo -e "\nChecking for ExecutionMetadata usage patterns..."
rg "ExecutionMetadata *{" --type go
echo -e "\nChecking for any direct struct field access..."
rg "\.(ExecutionID|JobID|SourcePeerID|TargetPeerID)" --type go
Length of output: 23096
pkg/models/messages/execution.go (2)
8-8: Document embedded metadata types.
The code embeds RoutingMetadata and ExecutionMetadata in multiple structures, but their purpose isn't immediately clear.
Let's check these types:
#!/bin/bash
# Look for the definitions of metadata types
rg -l "type (Routing|Execution)Metadata struct"Consider adding a package-level comment explaining these common embedded types and their significance in the messaging system.
Also applies to: 14-14, 18-19, 25-26
17-22: Consider pointer safety for RunResult fields.
The RunResult structure uses pointers for PublishResult and RunCommandResult, which implies these fields are optional. However, there's no documentation explaining when these fields might be nil.
Let's verify the usage of these fields:
Add documentation explaining the nullability:
+// RunResult contains the outcomes of both publish and run command operations
+// Either PublishResult or RunCommandResult or both may be nil depending on
+// the type of execution performed
type RunResult struct {
RoutingMetadata
ExecutionMetadata
PublishResult *models.SpecConfig
RunCommandResult *models.RunCommandResult
}✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient context about RunResult usage. The results show that RunResult is primarily used in callback mechanisms where execution results are passed through channels, and the fields are populated based on the type of operation performed (publish or run command).
Documentation needed for RunResult pointer fields
The review comment is correct. The search results show that RunResult is used in various contexts including test setups, callbacks, and execution flows. The pointer fields PublishResult and RunCommandResult are indeed used in a way where they can be nil, as evidenced by the analytics tracking and execution handling code.
Add documentation to clarify the nullability semantics:
+// RunResult contains the results of a compute node execution.
+// PublishResult is non-nil when the execution involved a publish operation
+// RunCommandResult is non-nil when the execution involved running a command
+// Both fields may be populated if the execution performed both operations
type RunResult struct {
RoutingMetadata
ExecutionMetadata
PublishResult *models.SpecConfig
RunCommandResult *models.RunCommandResult
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RunResult usage to understand field initialization patterns
rg -A 5 "RunResult.*{"
Length of output: 175
Script:
#!/bin/bash
# Let's try a more comprehensive search for RunResult usage
rg "RunResult" -B 2 -A 2
# Also search for struct definitions to understand the embedded types
rg "type (Routing|Execution)Metadata struct" -A 5
# Search for any functions that return RunResult
ast-grep --pattern 'func $$$($$$) RunResult {$$$}'
ast-grep --pattern 'func $$$($$$) *RunResult {$$$}'
Length of output: 6521
pkg/models/messages/node.go (2)
1-3: LGTM: Package name change improves clarity
The rename from requests to messages better represents the package's content, which includes both request and response types.
1-1: Verify package rename impact across codebase
Let's verify that all imports have been updated from requests to messages.
✅ Verification successful
Package rename has been properly implemented
The verification shows that:
- No references to the old package name
requestswere found in the codebase - The new package name
messagesis consistently imported across multiple packages - All imports are using the correct path
github.com/bacalhau-project/bacalhau/pkg/models/messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining imports of the old package name
rg '"pkg/.*requests"' --type go
# Search for imports of the new package to confirm the change
rg '"pkg/models/messages"' --type go
Length of output: 72
Script:
#!/bin/bash
# Let's try a broader search without quotes to catch all import variations
rg 'pkg/.*requests' --type go
# Search for the new package name without quotes
rg 'pkg/models/messages' --type go
# Also search for just the package name in import statements
rg '^import.*requests' --type go
# And search for the new package name in import statements
rg '^import.*messages' --type go
Length of output: 2994
pkg/node/ncl.go (2)
8-8: LGTM: Import addition aligns with PR objectives
The addition of the messages package import is consistent with the PR's goal of centralizing message definitions.
16-16: Verify type compatibility and consider constant location
The change to use messages.Heartbeat is aligned with the PR objectives. However:
- Let's verify the compatibility between the old and new types
- Consider whether
HeartbeatMessageTypeshould also move to the messages package for consistency
✅ Verification successful
Let me gather more information to specifically locate the old heartbeat type that was replaced and verify the message type constants pattern.
Based on the gathered information, I can now provide the final response:
Type compatibility verified, message type constant location is correct
The change from the old to new Heartbeat type is fully compatible:
- Both old and new types in
messagespackage have identical structure withNodeID(string) andSequence(uint64) - The
HeartbeatMessageTypeconstant's current location in theheartbeatpackage is appropriate because:- It's used by the heartbeat package's server implementation
- Follows the pattern where message types are defined in their handling packages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the structure of both Heartbeat types to ensure compatibility
# Check the old Heartbeat type structure
echo "Checking old Heartbeat type:"
ast-grep --pattern 'type Heartbeat struct { $$$ }'
# Check the new Heartbeat type structure
echo "Checking new Heartbeat type:"
ast-grep --pattern 'type Heartbeat struct { $$$ }'
# Look for other message types that might have been moved
echo "Checking for other message type constants:"
rg "MessageType.*=.*\".*\"" --type go
Length of output: 2582
Script:
#!/bin/bash
# Check git history for the old Heartbeat type location
git log -p | rg -B 5 -A 5 "type Heartbeat struct"
# Check for message type constant patterns across packages
echo "Message type constant patterns:"
rg "MessageType.*=.*" --type go -A 1
# Check imports in the registration file
echo "Checking imports in ncl.go:"
rg "^import \(" -A 20 pkg/node/ncl.go
Length of output: 3181
pkg/compute/callback_chain.go (2)
3-7: LGTM: Clean import changes
The import changes align with the PR objective of moving messaging models to pkg/models/messages.
24-44: LGTM: Consistent type updates across callback methods
The changes correctly update all callback method signatures to use the new message types from pkg/models/messages while maintaining the same delegation logic.
Let's verify that all callers have been updated to use the new message types:
✅ Verification successful
All callers consistently use the new message types from the messages package
The verification shows that all references to BidResult, RunResult, CancelResult, and ComputeError across the codebase are using the correct messages package types. This includes:
- Core compute package implementations (bidder, executor, callbacks)
- Test files and mocks
- External consumers in orchestrator and NATS proxy
- Type definitions in pkg/models/messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old types in the compute package
rg -g '!callback_chain.go' "BidResult|RunResult|CancelResult|ComputeError" pkg/compute/
# Search for any direct imports of compute package for message types
rg "\"github.com/bacalhau-project/bacalhau/pkg/compute\".*BidResult|RunResult|CancelResult|ComputeError"
Length of output: 7671
pkg/node/heartbeat/client.go (4)
10-10: LGTM: Import addition aligns with refactoring goals
The addition of the messages package import is consistent with the PR objective of centralizing message definitions.
Line range hint 21-29: LGTM: Constructor updated consistently
The NewClient function has been correctly updated to use messages.Heartbeat while maintaining its original behavior and error handling.
Line range hint 33-38: LGTM: Well-implemented backward compatibility
The SendHeartbeat method demonstrates good practices:
- Properly handles the type transition to messages.Heartbeat
- Maintains backward compatibility by publishing to both current and legacy topics
- Uses errors.Join for proper error aggregation
15-15: LGTM: Field type updated correctly
The field type change from local Heartbeat to messages.Heartbeat is consistent with the refactoring goals.
✅ Verification successful
Field type change is correctly implemented
The verification confirms that:
- The
messages.Heartbeatstruct inpkg/models/messages/node.gois the correct type being used for heartbeat messaging between nodes - The
types.Heartbeatstruct inpkg/config/types/compute.gois a different entity used for configuration purposes - All heartbeat messaging code consistently uses
messages.Heartbeat - The field type change aligns with the codebase's usage patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify no remaining references to the old Heartbeat type
rg -g '!*.md' -g '!*.mod' -g '!*.sum' 'type.*Heartbeat.*struct' --type go
Length of output: 496
Script:
#!/bin/bash
# Let's check the content of both Heartbeat structs to understand if they are related
echo "=== Content of pkg/models/messages/node.go Heartbeat struct ==="
rg -A 10 "type Heartbeat struct" pkg/models/messages/node.go
echo -e "\n=== Content of pkg/config/types/compute.go Heartbeat struct ==="
rg -A 10 "type Heartbeat struct" pkg/config/types/compute.go
# Let's also check where the Heartbeat from config/types is used
echo -e "\n=== Usage of config/types.Heartbeat ==="
rg "types\.Heartbeat" --type go
# And check where messages.Heartbeat is used
echo -e "\n=== Usage of messages.Heartbeat ==="
rg "messages\.Heartbeat" --type go
Length of output: 3228
pkg/models/messages/bidding.go (2)
1-4: LGTM!
Package declaration and imports are clean and follow Go conventions.
5-18: LGTM with verification needed.
The structure is clean and well-documented. However, let's verify the embedded types exist.
✅ Verification successful
Embedded types verified and properly defined ✅
Both RoutingMetadata and ExecutionMetadata are well-defined in metadata.go with appropriate fields:
RoutingMetadata: Contains source and target peer IDs for routingExecutionMetadata: Contains execution and job IDs with a helper constructor
The structure is clean, types are properly defined, and the embedding is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of RoutingMetadata and ExecutionMetadata types
# Description: These types are embedded but not defined in this file
echo "Searching for RoutingMetadata and ExecutionMetadata definitions..."
rg -t go "type (RoutingMetadata|ExecutionMetadata) struct" pkg/models/messages/
Length of output: 350
Script:
#!/bin/bash
# Let's verify the content of these types to ensure they are properly defined
echo "Checking the content of metadata.go..."
cat pkg/models/messages/metadata.go
Length of output: 519
pkg/compute/callback_mock.go (8)
3-7: LGTM: Clean import changes
The import changes correctly reference the new messages package location, aligning with the PR objective.
10-13: LGTM: Consistent type updates in CallbackMock struct
The handler fields have been correctly updated to use the new message types while maintaining the same structure and behavior.
17-20: LGTM: Clean OnBidComplete implementation
The method correctly implements the callback interface with proper nil checking and type updates.
24-27: LGTM: Clean OnCancelComplete implementation
The method correctly implements the callback interface with proper nil checking and type updates.
31-34: LGTM: Clean OnComputeFailure implementation
The method correctly implements the callback interface with proper nil checking and type updates.
38-41: LGTM: Clean OnRunComplete implementation
The method correctly implements the callback interface with proper nil checking and type updates.
Line range hint 44-44: LGTM: Interface compliance check maintained
The compile-time interface compliance check is correctly preserved.
Line range hint 1-44: Verify test coverage and usage of updated mock
Since this is a mock implementation, we should verify that all test files using this mock have been updated to use the new message types.
✅ Verification successful
All test files have been properly updated to use the new message types
Based on the verification results:
- The
CallbackMockis used inpkg/test/compute/setup_test.goand all its message types match the new implementation. - The search for usage of message types shows that all references to
messages.BidResult,messages.CancelResult,messages.ComputeError, andmessages.RunResultare consistent with the new implementation. - No outdated type references were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files using CallbackMock to ensure they've been updated
echo "Searching for test files using CallbackMock:"
rg -l "CallbackMock" --type go --type-add 'go:*.go' | grep "_test.go"
# Search for any remaining references to old types in test files
echo -e "\nChecking for potential outdated type usage in tests:"
rg -i "type:\s*(Bid|Cancel|Compute|Run)(Result|Error)" --type go
Length of output: 385
Script:
#!/bin/bash
# Let's examine the test file that uses CallbackMock
echo "Content of pkg/test/compute/setup_test.go:"
cat pkg/test/compute/setup_test.go
# Let's also check for any usage of the message types from the messages package
echo -e "\nChecking usage of messages package types:"
rg "messages\.(BidResult|CancelResult|ComputeError|RunResult)" --type go -A 2
Length of output: 17248
pkg/test/compute/mocks.go (4)
7-7: LGTM! Import change aligns with the PR objective.
The new import path correctly reflects the movement of messaging models to pkg/models/messages.
16-20: LGTM! Register method properly migrated to new message types.
The method signature and response creation have been correctly updated while maintaining the same behavior.
23-27: LGTM! UpdateInfo method properly migrated to new message types.
The method signature and response creation have been correctly updated while maintaining the same behavior.
11-13: LGTM! Handler signatures updated consistently.
All handler fields have been properly updated to use the new message types. Let's verify the interface compatibility across the codebase.
✅ Verification successful
Interface compatibility verified successfully
The verification shows:
- The
ManagementEndpointinterface inpkg/compute/types.gois already using the newmessagespackage types - All implementations (in mocks, proxies, and managers) have been updated as evidenced by the consistent imports
- No production code imports the old
requestspackage (only found in poetry.lock files and legacy config) - Wide adoption of the new
messagespackage across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all implementations of ManagementEndpoint interface use the new message types
# and there are no lingering uses of the old request types
# Check for any remaining references to the old request types
rg -l "requests\.(Register|UpdateInfo|UpdateResources)(Request|Response)"
# Check all implementations of ManagementEndpoint interface
ast-grep --pattern 'type $_ struct {
$$$
}
var _ compute.ManagementEndpoint = $_'
Length of output: 162
Script:
#!/bin/bash
# Let's try a different approach to verify the interface compatibility
# First, let's find the ManagementEndpoint interface definition
rg -A 10 "type ManagementEndpoint interface"
# Then check for any struct implementations and their handler methods
rg -l "func \(.*\) (Register|UpdateInfo|UpdateResources)\("
# Also check for any remaining imports of a 'requests' package
rg -l "\".*requests\""
# And verify the messages package usage
rg -l "\".*messages\""
Length of output: 2155
pkg/test/compute/bid_rejected_test.go (4)
13-13: LGTM: Import change aligns with package restructuring
The addition of the messages package import is consistent with the PR objective of centralizing message models.
40-40: LGTM: Error handling test preserved
The type change maintains the test's purpose of verifying error handling for non-existent execution IDs.
60-60: LGTM: State validation test maintained
The type change preserves the comprehensive testing of bid rejection handling across various invalid execution states.
32-32: LGTM: Type migration preserves test behavior
The change to use messages.BidRejectedRequest maintains the existing test logic while aligning with the new package structure.
Let's verify that all BidRejectedRequest usages have been migrated:
✅ Verification successful
All BidRejectedRequest references have been migrated to messages package
The verification shows complete migration of BidRejectedRequest:
- No occurrences of
compute.BidRejectedRequestfound - All 11 references consistently use
messages.BidRejectedRequestacross test, proxy, and core implementation files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify no remaining references to the old type
rg "compute\.BidRejectedRequest"
# Verify consistent usage of the new type
rg "messages\.BidRejectedRequest"
Length of output: 1423
pkg/test/compute/bid_accepted_test.go (4)
13-13: LGTM: Import addition aligns with package restructuring
The new import correctly supports the migration of messaging models to the dedicated package.
40-40: LGTM: Error handling test updated correctly
The type change maintains the test's error handling verification for non-existent execution IDs.
61-61: LGTM: State validation test updated appropriately
The type change preserves the test's validation of bid rejection for invalid execution states.
32-32: LGTM: Type migration maintains test functionality
The BidAcceptedRequest type has been correctly updated to use the new messages package.
✅ Verification successful
Type migration verified successfully
The BidAcceptedRequest struct is properly defined in pkg/models/messages/bidding.go with all necessary fields, confirming the correct type migration in the test file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the BidAcceptedRequest type exists in the new package
ast-grep --pattern 'type BidAcceptedRequest struct {
$$$
}'
Length of output: 383
pkg/nats/proxy/callback_proxy.go (3)
11-13: LGTM! Import organization follows Go conventions
The import changes maintain proper grouping while adding the new messages package, which aligns with the PR objective of transitioning message models.
Line range hint 71-72: LGTM! Implementation maintains integrity
The compile-time interface check and proxy behavior remain intact while transitioning to the new message types.
35-48: LGTM! Consistent method signature updates
The callback method signatures have been consistently updated to use the new message types while maintaining the same implementation logic.
Let's verify the interface compliance with the new types:
✅ Verification successful
Interface compliance verified - method signatures match perfectly ✅
The CallbackProxy implementation in pkg/nats/proxy/callback_proxy.go perfectly matches the Callback interface definition in pkg/compute/types.go. All method signatures are identical, using the same parameter types from the messages package:
OnBidComplete(ctx context.Context, result messages.BidResult)OnRunComplete(ctx context.Context, result messages.RunResult)OnCancelComplete(ctx context.Context, result messages.CancelResult)OnComputeFailure(ctx context.Context, err messages.ComputeError)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new message types match the compute.Callback interface definition
# Check the Callback interface definition in compute package
ast-grep --pattern 'type Callback interface {
$$$
OnBidComplete($_, $_)
OnRunComplete($_, $_)
OnCancelComplete($_, $_)
OnComputeFailure($_, $_)
$$$
}'
Length of output: 597
pkg/test/compute/ask_for_bid_test.go (3)
12-12: LGTM: Import change aligns with package restructuring.
The addition of the messages package import is consistent with the PR objective of moving messaging models to pkg/models/messages.
Line range hint 36-40: LGTM: Method signature update maintains functionality.
The verify method's parameter type change from compute.BidResult to messages.BidResult is consistent with the package restructuring while preserving the existing verification logic.
Line range hint 82-104: LGTM: Return type update preserves test behavior.
The runAskForBidTest method's return type change from compute.BidResult to messages.BidResult maintains the existing test functionality while aligning with the new package structure.
Let's verify that all BidResult type references have been updated consistently:
✅ Verification successful
All references to BidResult have been consistently updated to use messages.BidResult
The verification shows that all references to BidResult in the codebase are using the messages package. There are no remaining references to compute.BidResult, confirming that the type change has been consistently applied across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all BidResult type references have been updated
# Expected: No references to compute.BidResult should remain
# Search for any remaining references to compute.BidResult
rg "compute\.BidResult"
# Search for updated references to messages.BidResult for context
rg "messages\.BidResult"
Length of output: 1794
pkg/test/compute/ask_for_bid_pre_approved_test.go (2)
13-13: LGTM! Import change aligns with package reorganization.
The addition of the messages package import is consistent with the PR objective of moving messaging models to a centralized location.
Line range hint 81-86: LGTM! Verify consistent usage of messages package across related files.
The migration to messages.AskForBidRequest and messages.RoutingMetadata is correctly implemented and maintains the existing test behavior.
Let's verify that all related bid request usages have been migrated consistently:
✅ Verification successful
Migration to messages.AskForBidRequest is consistent across the codebase
The verification shows that all AskForBidRequest usages have been properly migrated to use the messages package. Every occurrence consistently uses messages.AskForBidRequest, including:
- Test files (pkg/test/compute/*)
- Core implementation files (pkg/compute/*)
- Orchestrator components (pkg/orchestrator/*)
- NATS proxy implementation (pkg/nats/proxy/*)
The type is properly defined in pkg/models/messages/bidding.go and all imports and usages align with the new package structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of messages package for bid requests
# Expected: All bid requests should use the messages package, not compute
# Check for any remaining references to compute.AskForBidRequest
rg -l "compute\.AskForBidRequest"
# Check for adoption of messages.AskForBidRequest
rg -l "messages\.AskForBidRequest"
Length of output: 363
Script:
#!/bin/bash
# Let's examine the actual usage in these files to determine if there's inconsistency
# Looking for both old and new package usage patterns with context
# Search for AskForBidRequest usage with context
rg "AskForBidRequest" -B 2 -A 2
# Also check imports to see package usage
rg "import.*compute|import.*messages" --type go
Length of output: 4848
pkg/nats/proxy/management_handler.go (4)
14-14: LGTM! Import change aligns with package reorganization.
The addition of the messages package import is consistent with the PR objective of moving messaging models to pkg/models/messages.
82-83: LGTM! Clean migration to messages package.
The method signature and request type updates maintain the same functionality while using the new package structure.
93-94: LGTM! Consistent package migration.
The changes follow the same pattern as processRegistration, maintaining consistency across the codebase.
104-105: LGTM! Package migration complete.
The changes maintain consistency with other methods while properly transitioning to the new package structure.
Let's verify that all references to the old package have been updated:
✅ Verification successful
Based on the extensive search results, I can see that the migration from the old requests package to the new messages package has been completed consistently across the codebase. The search results show:
- No remaining imports of the old
requestspackage - All references to the old request/response types have been updated to use the new
messagespackage - The changes in
management_handler.gofollow the same pattern as other files in using the newmessagespackage
Package migration verified successfully. No remaining references to the old package were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old requests package
rg "\"github.com/bacalhau-project/bacalhau/pkg/compute/requests\""
# Search for any remaining type references that might have been missed
rg "requests\.(Register|UpdateInfo|UpdateResources)(Request|Response)"
Length of output: 139
Script:
#!/bin/bash
# Let's try a broader search to catch any remaining references to the requests package
rg -i "requests\." -A 2 -B 2
# Also search for the specific message types without the package prefix
rg "(Register|UpdateInfo|UpdateResources)(Request|Response)" -A 2 -B 2
# Let's also check imports in the entire codebase
rg "^import \(" -A 10
Length of output: 505714
pkg/nats/proxy/management_proxy.go (3)
14-14: LGTM: Import change aligns with package restructuring
The addition of the messages package import is consistent with the PR objective of centralizing message definitions.
22-22: LGTM: Type constraints properly updated
The interface type constraints have been correctly updated to use the new message types, maintaining type safety through union types.
Also applies to: 26-26
50-52: Verify all callers have been updated to use new message types
The method signatures have been correctly updated to use the new message types. Let's verify that all callers have been updated accordingly.
Also applies to: 68-70, 85-87
✅ Verification successful
Let me check the actual usage context in these files to verify if they're using the old types or have been properly updated.
All callers are properly using the new message types
The verification shows that all callers are correctly using the messages package types. Every occurrence of RegisterRequest, UpdateInfoRequest, and UpdateResourcesRequest is properly prefixed with messages. package. This includes:
- Interface definition in
pkg/compute/types.go - Implementation in
pkg/node/manager/node_manager.go - Client usage in
pkg/compute/management_client.go - Handler processing in
pkg/nats/proxy/management_handler.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old request types
rg -l "RegisterRequest|UpdateInfoRequest|UpdateResourcesRequest" --type go | grep -v "pkg/models/messages"
# Search for direct usage of these methods to verify parameter types
ast-grep --pattern 'Register($_, $_)'
ast-grep --pattern 'UpdateInfo($_, $_)'
ast-grep --pattern 'UpdateResources($_, $_)'
Length of output: 441
Script:
#!/bin/bash
# Check the actual usage in the files found
echo "=== management_handler.go ==="
rg "RegisterRequest|UpdateInfoRequest|UpdateResourcesRequest" pkg/nats/proxy/management_handler.go -C 2
echo "=== node_manager.go ==="
rg "RegisterRequest|UpdateInfoRequest|UpdateResourcesRequest" pkg/node/manager/node_manager.go -C 2
echo "=== management_client.go ==="
rg "RegisterRequest|UpdateInfoRequest|UpdateResourcesRequest" pkg/compute/management_client.go -C 2
echo "=== compute/types.go ==="
rg "RegisterRequest|UpdateInfoRequest|UpdateResourcesRequest" pkg/compute/types.go -C 2
Length of output: 3371
pkg/test/compute/cancel_test.go (3)
17-17: LGTM: Import change aligns with package reorganization
The addition of the messages package import is consistent with the PR objective of moving messaging models to their new location.
40-40: LGTM: Request type updates maintain test integrity
The changes correctly update the request types to use the new messages package while preserving the original test logic and validation flow.
Also applies to: 48-50
93-93: LGTM: Comprehensive state handling preserved
The request type updates maintain the thorough test coverage across different execution states, ensuring robust validation of the cancellation logic.
Also applies to: 123-123
pkg/nats/proxy/compute_proxy.go (3)
53-55: LGTM! Consistent migration to new message types.
The method signatures have been consistently updated to use the new messages package types, maintaining interface compatibility while aligning with the PR objective of centralizing message definitions.
Also applies to: 62-64, 71-73, 81-83, 90-93
53-93: Verify that all callers have been updated to use the new message types.
While the changes look correct, we should ensure all code calling these proxy methods has been updated to use the new message types to prevent compilation errors.
✅ Verification successful
Based on the search results, I can generate the final response now:
All callers have been updated to use the new message types from the messages package
The verification shows that:
- No references to old compute message types were found
- All code consistently uses the new
messagespackage for request/response types - The migration appears complete across all relevant packages:
- pkg/compute/*
- pkg/nats/proxy/*
- pkg/orchestrator/*
- pkg/test/compute/*
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to old compute message types
echo "Checking for old compute package message usage..."
rg --type go "compute\.(?:AskForBid|BidAccepted|BidRejected|CancelExecution|ExecutionLogs)(?:Request|Response)"
# Search for new message type usage to confirm migration
echo "Verifying new message package usage..."
rg --type go "messages\.(?:AskForBid|BidAccepted|BidRejected|CancelExecution|ExecutionLogs)(?:Request|Response)"
Length of output: 9400
15-15: Verify if the compute package import is still needed.
Since message types are being moved to the new messages package, we should verify if the compute package import is still required.
pkg/test/compute/setup_test.go (4)
23-23: LGTM: Import addition aligns with PR objectives.
The addition of the messages package import is consistent with the PR's goal of moving messaging models.
43-45: LGTM: Channel type migrations are correct.
The channel type changes from compute package to messages package are consistent and properly typed.
122-130: LGTM: Callback handlers properly migrated.
The callback handlers have been correctly updated to use the new message types while maintaining the same channel operation semantics.
160-162: LGTM: Verify bid request handling across the codebase.
The migration to messages package types is correct. The method signature and request structure are properly updated.
Let's verify that all bid request handlers are consistently using the new types:
Also applies to: 176-176
✅ Verification successful
Migration to messages.BidResult is complete and consistent
The verification shows that:
- All bid-related types are properly using the new
messagespackage - The type definition exists only in
pkg/models/messages/bidding.go - All references across the codebase (compute package, tests, proxies, callbacks) consistently use the new
messages.BidResultandmessages.AskForBidRequesttypes - No legacy
compute.BidResulttype definitions remain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old compute.BidResult type
rg -l "compute\.(?:BidResult|AskForBidRequest)"
# Search for consistent usage of the new message types
rg -l "messages\.(?:BidResult|AskForBidRequest)"
Length of output: 571
Script:
#!/bin/bash
# Let's check the actual content of these files to verify the type usage
rg "messages\.(?:BidResult|AskForBidRequest)" -A 2 -B 2
# Also specifically check compute package for any old type definitions
ast-grep --pattern 'type BidResult struct {
$$$
}'
Length of output: 9970
pkg/compute/endpoint.go (4)
Line range hint 91-122: LGTM! Clean transition to messages package
The changes properly integrate the new message types while maintaining the existing logic and error handling.
Line range hint 124-146: LGTM! Consistent with BidAccepted pattern
The changes maintain consistency with the BidAccepted implementation while properly handling the rejection justification.
Line range hint 181-188: LGTM! Clean adapter implementation
The method properly adapts the new message type to the existing log server interface.
Line range hint 77-83: Consider updating BidderRequest to use messages package
While the method signature has been updated to use the new messages package, the BidderRequest type in the RunBidding call is still using a local type. This seems inconsistent with the PR's objective of centralizing message types.
Let's check if BidderRequest is defined locally:
Consider moving BidderRequest to the messages package for consistency.
pkg/compute/management_client.go (5)
14-14: LGTM! Import change aligns with package reorganization.
The import statement correctly reflects the move of messaging models to the new package location.
85-87: LGTM! Type reference updated correctly.
The RegisterRequest type reference has been properly updated to use the new messages package while maintaining the same functionality.
113-115: LGTM! Type reference updated correctly.
The UpdateInfoRequest type reference has been properly updated to use the new messages package while maintaining the same functionality.
Line range hint 129-133: LGTM! Type reference updated correctly.
The UpdateResourcesRequest type reference and struct initialization have been properly updated to use the new messages package while maintaining the same functionality.
14-14: Verify complete removal of old package references.
Let's ensure there are no remaining references to the old requests package.
✅ Verification successful
Since we found no references to the old package "pkg/models/requests", let me verify one more aspect to ensure completeness by checking for any direct references to "requests" in import statements:
Based on the analysis of the imports across the codebase, I can see that there are no remaining references to the old "pkg/models/requests" package. The migration to "pkg/models/messages" appears to be complete. Let me generate the final response:
Migration to messages package is complete
No remaining references to the old pkg/models/requests package were found in the codebase. The migration to pkg/models/messages has been successfully completed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old requests package
rg -l "pkg/models/requests" || echo "No references found to old package"
Length of output: 106
Script:
#!/bin/bash
# Search for any remaining import statements referencing 'requests'
ast-grep --pattern 'import (
$$$
"github.com/bacalhau-project/bacalhau/pkg/models/requests"
$$$
)'
Length of output: 454485
pkg/orchestrator/planner/compute_forwarder.go (5)
11-11: LGTM: Import change aligns with package restructuring.
The addition of the messages package import while retaining the compute package is appropriate for the refactoring.
Line range hint 84-91: LGTM: Request type migration is correct.
The request type has been properly migrated to use the new messages package while maintaining the same structure and functionality.
Line range hint 110-116: LGTM: Request type migration is correct.
The BidAcceptedRequest has been properly migrated to the messages package while preserving the existing functionality.
Line range hint 131-137: LGTM: Request type migration is correct.
The BidRejectedRequest has been properly migrated to the messages package while maintaining the same structure.
Line range hint 153-160: LGTM: Request type migration is correct.
The CancelExecutionRequest has been properly migrated to the messages package while preserving the existing functionality.
Let's verify that all compute message types have been consistently migrated:
✅ Verification successful
Message types have been consistently migrated to the messages package
The verification results confirm that:
- All message types (AskForBidRequest, BidAcceptedRequest, BidRejectedRequest, CancelExecutionRequest, RoutingMetadata) are properly defined in the messages package
- All usages of these types across the codebase reference them from the messages package
- No legacy references to these types from the compute package remain
- The compute package imports are only for other functionality, not for these message types
The migration has been completed consistently throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify no remaining compute package message types
# Test: Search for any remaining compute package message types that should have been migrated
rg "compute\.(AskForBidRequest|BidAcceptedRequest|BidRejectedRequest|CancelExecutionRequest|RoutingMetadata)"
Length of output: 111
Script:
#!/bin/bash
# Let's verify the migration by checking for any remaining references to the compute package types
# and also confirm the new message types exist in the messages package
# Check for any remaining compute package imports
rg "\".*compute\"" --type go
# Check for existence of these message types in messages package
rg "type (AskForBidRequest|BidAcceptedRequest|BidRejectedRequest|CancelExecutionRequest|RoutingMetadata) struct" --type go
# Check for usage of these types from messages package
rg "messages\.(AskForBidRequest|BidAcceptedRequest|BidRejectedRequest|CancelExecutionRequest|RoutingMetadata)" --type go
Length of output: 11003
pkg/orchestrator/endpoint.go (2)
17-17: LGTM: Clean import addition
The new messages package import is correctly placed and aligns with the PR objective of moving messaging models.
Line range hint 229-234: LGTM: Clean type migration to messages package
The migration from compute to messages package types is clean and maintains the existing behavior.
Let's verify the integration points:
✅ Verification successful
Migration to messages package is complete and consistent
The verification confirms that:
- All
ExecutionLogsmethod signatures across the codebase (ComputeProxy,BaseEndpoint, interfaces) usemessages.ExecutionLogsRequest - No remaining references to
compute.ExecutionLogsRequestexist - The type is properly defined in
pkg/models/messages/logging.go - All integration points (callers and implementations) are aligned with the new type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all related components are updated to use messages.ExecutionLogsRequest
# Test 1: Check if computeProxy.ExecutionLogs method signature is updated
ast-grep --pattern 'type Endpoint interface {
$$$
ExecutionLogs(ctx context.Context, request messages.ExecutionLogsRequest) (<-chan *concurrency.AsyncResult[models.ExecutionLog], error)
$$$
}'
# Test 2: Search for any remaining references to compute.ExecutionLogsRequest
rg "compute\.ExecutionLogsRequest"
# Test 3: Search for any remaining references to compute.RoutingMetadata
rg "compute\.RoutingMetadata"
Length of output: 1681
Script:
#!/bin/bash
# Let's verify the integration points with a different approach
# Check for any references to ExecutionLogs method calls
rg "ExecutionLogs\(" -A 2
# Check for any imports of the compute package
rg "\".*compute\""
# Check for any remaining references to ExecutionLogsRequest in any form
rg "ExecutionLogsRequest"
Length of output: 6573
pkg/orchestrator/planner/utils_test.go (2)
12-12: LGTM! Import changes align with package reorganization.
The addition of the messages package import is consistent with the PR objective of moving messaging models from pkg/compute to pkg/models/messages.
Line range hint 209-235: LGTM! Type migrations are consistent.
The ComputeRequestMatcher has been correctly updated to use the new message types while preserving all existing functionality and logic.
Let's verify that all compute request types have been migrated consistently:
✅ Verification successful
Type migration is complete and consistent
The verification confirms that:
- No references to the old compute package request types remain in the codebase
- All request types (AskForBidRequest, BidAcceptedRequest, BidRejectedRequest, CancelExecutionRequest) consistently use the new
messagespackage across all usages including:- Test files
- Compute forwarder
- NATS proxy
- Compute endpoint implementations
- Interface definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that no old compute package request types remain in the codebase
# and that all message types are from the new package
# Check for any remaining compute package request types
echo "Checking for any remaining compute package request types..."
rg "compute\.(AskForBidRequest|BidAcceptedRequest|BidRejectedRequest|CancelExecutionRequest)"
# Verify usage of new message types
echo "Verifying usage of new message types..."
rg "messages\.(AskForBidRequest|BidAcceptedRequest|BidRejectedRequest|CancelExecutionRequest)"
Length of output: 6133
pkg/node/heartbeat/server.go (8)
17-17: LGTM: Import addition aligns with PR objective.
The addition of the messages package import is consistent with the goal of centralizing message definitions.
40-40: LGTM: Correct type parameter update.
The legacySubscriber field type is properly updated to use messages.Heartbeat.
48-48: LGTM: Proper type embedding update.
The embedded Heartbeat type is correctly updated to messages.Heartbeat.
53-53: LGTM: Constructor properly updated.
The NewPubSub instantiation correctly uses messages.Heartbeat as its type parameter.
218-218: LGTM: Method signature properly updated.
The Handle method signature is correctly updated to accept messages.Heartbeat.
232-235: LGTM: Type assertions properly updated.
The type assertion and error message correctly reference messages.Heartbeat.
241-241: LGTM: Interface assertion properly updated.
The interface assertion correctly uses messages.Heartbeat as the type parameter.
Line range hint 1-241: Verify complete migration of Heartbeat type.
Let's ensure no references to the old Heartbeat type remain in the codebase.
✅ Verification successful
Let me verify the usage of the remaining Heartbeat type in pkg/config/types/compute.go to understand its relationship with messages.Heartbeat.
The findings show that types.Heartbeat is a different type used for configuration purposes, containing intervals and durations, while messages.Heartbeat (the one in the reviewed code) is used for heartbeat message communication. These are separate concerns and should coexist.
Migration verified successfully - no issues found
The remaining Heartbeat type in pkg/config/types/compute.go is a configuration structure that defines heartbeat intervals and is unrelated to the message type migration in the reviewed code. The changes in the reviewed file correctly use messages.Heartbeat for communication while preserving the separate configuration type. This is a proper separation of concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old Heartbeat type in the codebase
# Exclude the messages package itself and test files
# Look for type definitions of Heartbeat
echo "Checking for Heartbeat type definitions:"
rg "type\s+Heartbeat\s+struct" --type go --glob '!pkg/models/messages/**' --glob '!**/*_test.go'
# Look for Heartbeat type usage without messages prefix
echo "Checking for Heartbeat type usage without messages prefix:"
rg "\bHeartbeat\b" --type go --glob '!pkg/models/messages/**' --glob '!**/*_test.go' | grep -v "messages\.Heartbeat"
Length of output: 1976
Script:
#!/bin/bash
# Check the content of the Heartbeat type in config/types
echo "Content of Heartbeat type in config/types:"
cat pkg/config/types/compute.go
# Check if there are any imports of messages package in this file
echo -e "\nImports of messages package:"
rg "messages" pkg/config/types/compute.go
# Check the usage of this Heartbeat type
echo -e "\nUsage of types.Heartbeat:"
rg "types\.Heartbeat" --type go
Length of output: 2694
pkg/orchestrator/callback.go (5)
13-13: LGTM: Import changes align with package reorganization.
The addition of the messages package import is consistent with the PR objective of moving messaging models to a dedicated package.
39-39: LGTM: Type change maintains method behavior.
The parameter type change from compute.BidResult to messages.BidResult is consistent with the package reorganization while preserving the existing functionality and robust error handling.
99-99: LGTM: Type change preserves complex execution state handling.
The parameter type change from compute.RunResult to messages.RunResult maintains the existing robust state management and transaction handling, particularly for long-running jobs.
173-173: LGTM: Type change maintains error handling integrity.
The parameter type change from compute.ComputeError to messages.ComputeError preserves the comprehensive error handling and state management logic.
39-39: Verify consistent usage of new message types.
Let's ensure all related files have been updated to use the new message types from the messages package.
Also applies to: 99-99, 168-168, 173-173
✅ Verification successful
Let me check the actual usage context in these files to ensure the types are used consistently.
Based on the comprehensive search results, I can now provide the final response:
All message types are consistently used from the messages package
The verification shows that all references to BidResult, RunResult, CancelResult, and ComputeError types are correctly using the messages package. There are no remaining references to these types from the old compute package, and all callback implementations across the codebase (Callback, CallbackProxy, CallbackMock, ChainedCallback) consistently use the new message types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old types in compute package
# and verify that all necessary files have been updated.
echo "Checking for old type references..."
rg -l "compute\.(?:BidResult|RunResult|CancelResult|ComputeError)" --type go
echo "Verifying message type usage consistency..."
rg -l "messages\.(?:BidResult|RunResult|CancelResult|ComputeError)" --type go
Length of output: 671
Script:
#!/bin/bash
# Check the actual usage context of these types in the files
echo "Checking BidResult usage..."
rg "BidResult" -B 2 -A 2 --type go
echo -e "\nChecking RunResult usage..."
rg "RunResult" -B 2 -A 2 --type go
echo -e "\nChecking CancelResult usage..."
rg "CancelResult" -B 2 -A 2 --type go
echo -e "\nChecking ComputeError usage..."
rg "ComputeError" -B 2 -A 2 --type go
Length of output: 22302
pkg/compute/executor_buffer.go (2)
16-16: LGTM: Import change aligns with package reorganization.
The addition of the messages package import is consistent with the PR objective of centralizing message types.
Line range hint 98-104: LGTM: Error handling updates align with new message types.
The migration to messages.ComputeError and associated types is implemented correctly. Let's verify consistent usage across the codebase.
✅ Verification successful
ComputeError migration is complete and consistent across the codebase
The verification shows:
- Single source of truth:
ComputeErroris defined only inpkg/models/messages/execution.go - All usages consistently reference
messages.ComputeError - No unmigrated or standalone
ComputeErrorinstances found (except for the type definition and analytics fields which are unrelated)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of messages.ComputeError across codebase
# Expected: All ComputeError usages should be from messages package
# Check for any remaining direct ComputeError type definitions
echo "Checking for remaining ComputeError type definitions..."
rg "type\s+ComputeError\s+struct" --type go
# Check for consistent usage of messages.ComputeError
echo "Checking messages.ComputeError usage..."
rg "messages\.ComputeError" --type go
# Check for potential missed migrations
echo "Checking for potential unmigrated ComputeError usages..."
rg "ComputeError[^.]" --type go
Length of output: 3714
pkg/orchestrator/planner/compute_forwarder_test.go (1)
17-17: LGTM! Import aligns with the package reorganization.
The addition of the messages package import is consistent with the PR's objective of centralizing message definitions.
pkg/node/manager/node_manager.go (5)
13-13: LGTM: Import change aligns with PR objective
The addition of the messages package import is consistent with the goal of moving messaging models from pkg/compute to pkg/models/messages.
81-81: LGTM: Clean type migration for Register method
The changes correctly migrate the Register method to use the new message types while preserving the existing logic flow and error handling.
Also applies to: 86-89, 94-97, 109-111
116-116: LGTM: Clean type migration for UpdateInfo method
The changes successfully migrate the UpdateInfo method to use the new message types while maintaining the existing behavior.
Also applies to: 120-123, 131-134, 148-150
156-156: LGTM: Clean type migration for UpdateResources method
The changes successfully migrate the UpdateResources method to use the new message types while preserving the existing functionality.
Also applies to: 164-164, 175-175
81-81: Verify all callers are updated to use the new message types
Let's verify that all callers of these methods have been updated to use the new types from the messages package.
Also applies to: 116-116, 156-156
✅ Verification successful
Let me check the actual content of these files to determine if there are any remaining references to the old types.
Let me try a different search pattern to check the usage of both old and new types.
Based on the search results, I can now generate the final response since I have enough information to verify the changes:
All callers have been properly updated to use the new message types
The codebase scan shows that:
- All references to the new
messagespackage types are consistently used across the codebase - The old
computepackage types are only referenced in:- Interface definitions and mocks
- Type assertions and implementations
- No actual usage of old request/response types found in method calls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old types
rg -l "compute\.(?:Register|UpdateInfo|UpdateResources)(?:Request|Response)"
# Search for usage of the new types to confirm migration
rg -l "messages\.(?:Register|UpdateInfo|UpdateResources)(?:Request|Response)"
Length of output: 364
Script:
#!/bin/bash
# Check the actual usage context in each file
echo "=== pkg/test/compute/mocks.go ==="
rg "compute\.(Register|UpdateInfo|UpdateResources)(Request|Response)" pkg/test/compute/mocks.go -A 2
echo -e "\n=== pkg/node/manager/node_manager.go ==="
rg "compute\.(Register|UpdateInfo|UpdateResources)(Request|Response)" pkg/node/manager/node_manager.go -A 2
echo -e "\n=== pkg/nats/proxy/management_proxy.go ==="
rg "compute\.(Register|UpdateInfo|UpdateResources)(Request|Response)" pkg/nats/proxy/management_proxy.go -A 2
echo -e "\n=== pkg/nats/proxy/management_handler.go ==="
rg "compute\.(Register|UpdateInfo|UpdateResources)(Request|Response)" pkg/nats/proxy/management_handler.go -A 2
echo -e "\n=== pkg/compute/management_client.go ==="
rg "compute\.(Register|UpdateInfo|UpdateResources)(Request|Response)" pkg/compute/management_client.go -A 2
echo -e "\n=== pkg/compute/mocks.go ==="
rg "compute\.(Register|UpdateInfo|UpdateResources)(Request|Response)" pkg/compute/mocks.go -A 2
echo -e "\n=== pkg/compute/types.go ==="
rg "compute\.(Register|UpdateInfo|UpdateResources)(Request|Response)" pkg/compute/types.go -A 2
Length of output: 1367
Script:
#!/bin/bash
# Check for any remaining references to compute package types
echo "=== Old types (compute package) ==="
rg -A 3 "compute\." --type go
echo -e "\n=== New types (messages package) ==="
rg -A 3 "messages\." --type go
Length of output: 81192
pkg/node/heartbeat/heartbeat_test.go (2)
21-21: LGTM: Import addition aligns with PR objective.
The addition of the messages package import is consistent with moving messaging models to pkg/models/messages.
62-62: Verify test coverage after message type change.
The registration of messages.Heartbeat{} looks correct, but let's verify the test coverage to ensure no regression.
✅ Verification successful
Let me gather more information to verify the message type change properly.
Let me try one more verification to locate the messages package and check the Heartbeat type definition.
Registration of messages.Heartbeat is correct and consistent with the codebase
The verification confirms that:
- The Heartbeat type is properly defined in
pkg/models/messages/node.go - The registration in the test uses the correct import path
messages.Heartbeat{} - All tests are properly handling the type, with no errors related to the registration
- The TODO comment in
types.gois unrelated to the type registration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage and usage of messages.Heartbeat
# Test 1: Check if there are any remaining references to the old Heartbeat type
rg -l "Heartbeat{}" --type go
# Test 2: Verify all tests are passing in this package
rg -l "func Test.*" pkg/node/heartbeat/
# Test 3: Check for any TODOs or FIXMEs related to heartbeat messages
rg -i "TODO|FIXME.*heart.*" pkg/node/heartbeat/
Length of output: 346
Script:
#!/bin/bash
# Check for all usages of Heartbeat type and its registration
rg "Heartbeat" pkg/node/heartbeat/ -A 2 -B 2
# Check for any test failures or errors related to heartbeat
rg "Error|Failed.*[Hh]eartbeat" pkg/node/heartbeat/
# Check the messages package for Heartbeat type definition
rg "Heartbeat" pkg/node/messages/ -A 2 -B 2
Length of output: 20363
Script:
#!/bin/bash
# Find the messages package location
fd messages
# Check for Heartbeat type definition in any Go file
ast-grep --pattern 'type Heartbeat struct {
$$$
}'
Length of output: 1136
pkg/compute/bidder_test.go (4)
19-19: LGTM: Import change aligns with package restructuring.
The addition of the messages package import is consistent with the PR objective of moving messaging models to a dedicated package.
71-74: LGTM: Clean migration to messages package.
The change from compute.AskForBidRequest to messages.AskForBidRequest is a clean transition that maintains the existing test behavior while aligning with the new package structure.
173-176: LGTM: Consistent type migration.
The change to messages.AskForBidRequest maintains consistency with the previous test case while preserving all test scenarios and error handling.
Line range hint 285-289: LGTM: Type assertion properly updated.
The type assertion change from compute.BidResult to messages.BidResult is correctly implemented with proper type checking.
pkg/compute/mocks.go (5)
13-13: LGTM: Import change aligns with PR objective
The addition of the messages package import is consistent with the goal of moving messaging models to pkg/models/messages.
41-44: LGTM: Consistent type reference updates in MockEndpoint
The method signatures have been correctly updated to use types from the messages package while maintaining the original interface contract. The ExecutionLogs method appropriately retains models.ExecutionLog for its return type.
Also applies to: 56-59, 71-74, 86-89, 101-101
190-190: LGTM: Consistent type updates in MockCallback methods
All callback method signatures have been properly updated to use types from the messages package while maintaining the original interface contract.
Also applies to: 202-202, 214-214, 226-226
261-264: LGTM: Consistent type updates in MockManagementEndpoint
All management endpoint method signatures have been correctly updated to use types from the messages package while maintaining pointer receivers where appropriate.
Also applies to: 276-279, 291-294
Line range hint 1-2: Verify source file update and mock regeneration
Since this is a generated mock file (as indicated by the header comment), please ensure that:
- The source file (types.go) has been updated with the new message type imports
- The mocks were regenerated using the correct gomock version
✅ Verification successful
The verification results show that:
- The source file
types.gocontains the correctgo:generate mockgendirective - The imports in
types.gohave been properly updated to use the message types from the new package location (pkg/models/messages) - The mock generation command in the directive matches the generated file header
Mock generation setup is correct and source file is properly updated
The source file has been correctly updated with the new message type imports and contains the proper mock generation directive. The generated mock file's header correctly references types.go as its source, indicating proper regeneration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify source file updates and presence of go:generate directive
rg -l "go:generate mockgen" pkg/compute/
cat pkg/compute/types.go
Length of output: 3174
pkg/compute/bidder.go (5)
16-16: LGTM: Import addition aligns with package restructuring.
The addition of the messages package import is consistent with the PR's objective of centralizing message definitions.
63-68: LGTM: Clean transition to new message types.
The refactoring correctly moves from local types to the centralized message types while maintaining the same functionality and structure.
91-97: LGTM: Consistent usage of new message types in RunBidding.
The changes properly integrate the new message types for both metadata and error handling while maintaining the original functionality.
Also applies to: 106-111
131-137: LGTM: Comprehensive update of message types in handleBidResult.
The changes successfully migrate all messaging-related structures to the new package while preserving the existing behavior and error handling patterns.
Also applies to: 146-150, 153-164
Line range hint 1-324: Verify consistent usage of message types across the codebase.
While the changes in this file are well-structured, let's verify that all related files are consistently using the new message types.
✅ Verification successful
Based on the results, I can now generate the final response since we have sufficient information:
Message types migration appears complete and consistent
The verification shows:
- No remaining direct type definitions of
BidResult,RoutingMetadata,ExecutionMetadata, orComputeErroroutside the messages package - Consistent import and usage of the messages package across relevant components:
- Compute subsystem (
pkg/compute/*) - Orchestrator subsystem (
pkg/orchestrator/*) - Node management (
pkg/node/*) - NATS proxies (
pkg/nats/proxy/*) - Test utilities (
pkg/test/compute/*)
- Compute subsystem (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct usage of old types or inconsistent usage of new message types
# Look for any remaining direct usage of old types
echo "Checking for potential missed migrations to messages package..."
rg -g '!vendor/**' -g '!pkg/models/messages/**' "type.*(?:BidResult|RoutingMetadata|ExecutionMetadata|ComputeError)\s+struct"
# Verify consistent import of messages package
echo "Verifying consistent import of messages package..."
rg -g '*.go' -l "github.com/bacalhau-project/bacalhau/pkg/models/messages"
Length of output: 1439
pkg/compute/executor.go (5)
14-14: LGTM: Import change aligns with package restructuring.
The addition of the messages package import is consistent with the PR objective of centralizing message definitions.
426-428: LGTM: Callback refactored to use centralized message types.
The OnRunComplete callback now correctly uses the new messages.RunResult type while maintaining all necessary metadata.
470-472: LGTM: Cancel callback updated to use centralized message types.
The OnCancelComplete callback now correctly uses the new messages.CancelResult type while maintaining all necessary metadata.
494-496: LGTM: Error handling updated to use centralized message types.
The OnComputeFailure callback now correctly uses the new messages.ComputeError type while maintaining all necessary metadata.
Line range hint 426-496: Verify complete migration to centralized message types.
Let's verify that all references to these message types have been consistently updated across the codebase:
✅ Verification successful
All message type references have been properly migrated to the centralized package
The verification shows:
- No local definitions of RunResult, CancelResult, or ComputeError types found outside the messages package
- All callback implementations consistently use the messages package types
- The messages package is properly imported across all relevant files
- All usages of OnRunComplete, OnCancelComplete, and OnComputeFailure are using the correct message types from the messages package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining local message type definitions or usages
# that should have been migrated to the messages package.
# Test 1: Look for any remaining RunResult, CancelResult, or ComputeError type definitions
echo "Checking for remaining local message type definitions..."
rg -g '!pkg/models/messages/*' '(type\s+(RunResult|CancelResult|ComputeError)|struct\s+\{.*?(RunResult|CancelResult|ComputeError))'
# Test 2: Verify all callbacks are using the new message types
echo "Verifying callback implementations..."
rg 'OnRunComplete|OnCancelComplete|OnComputeFailure'
# Test 3: Check imports of the new messages package
echo "Checking messages package imports..."
rg '"github.com/bacalhau-project/bacalhau/pkg/models/messages"'
Length of output: 12087
pkg/compute/types.go (4)
17-26: Interface methods updated to use new messages types
The method signatures in the Endpoint interface have been updated to use types from the messages package. This change enhances consistency and centralizes message definitions.
40-43: Callback interface now utilizes messages types
The Callback interface methods have been updated:
OnBidCompleteOnRunCompleteOnCancelCompleteOnComputeFailure
These methods now accept parameters from the messages package, improving clarity and maintainability.
9-9: Verify the removal of old request and response types
The import statement now includes the messages package:
"github.com/bacalhau-project/bacalhau/pkg/models/messages"Ensure that all references to the old request and response types from pkg/compute have been updated to use the new types from the messages package.
Run the following script to check for any remaining references to the old types:
✅ Verification successful
Based on the search results, I can generate the final response as we have comprehensive information about the usage of these types in the codebase.
All request and response types are correctly using the messages package
The search results confirm that all references to AskForBidRequest, BidAcceptedRequest, BidRejectedRequest, CancelExecutionRequest, BidResult, RunResult, CancelResult, and ComputeError are properly using the messages package. These types are:
- Defined in
pkg/models/messages/ - Used consistently across the codebase with the
messagespackage prefix - No legacy usages found in
pkg/computewithout themessagespackage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for old request and response types in the codebase.
# Test: Find any usage of the old request and response types.
rg '\b(AskForBidRequest|BidAcceptedRequest|BidRejectedRequest|CancelExecutionRequest|BidResult|RunResult|CancelResult|ComputeError)\b' --type go
Length of output: 11980
50-54: Update implementations of ManagementEndpoint interface
The ManagementEndpoint interface methods now use the new messages types. Ensure that all implementations of this interface are updated to match the new method signatures.
Run the following script to identify implementations that may need updating:
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ct/bacalhau into refactor-messaging-models
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pkg/test/compute/setup_test.go (1)
Line range hint
158-174: Consider improving error handling in timeout case.While the changes to use the new message types are correct, the error case could be improved.
Consider adding an error field to the returned BidResult in the timeout case:
case <-time.After(5 * time.Second): s.FailNow("did not receive a bid response") - return messages.BidResult{} + return messages.BidResult{ + Accepted: false, + Error: "timeout waiting for bid response", + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pkg/test/compute/setup_test.go(6 hunks)
🔇 Additional comments (4)
pkg/test/compute/setup_test.go (4)
23-23: LGTM!
The new import for the messages package is correctly placed and necessary for the type changes.
43-45: LGTM!
The channel types are consistently updated to use the new messages package, aligning with the PR's objective of centralizing message definitions.
106-108: Duplicate channel initialization issue persists.
The issue highlighted in the previous review still exists where completedChannel is initialized twice.
120-128: LGTM!
The callback handlers are correctly updated to use the new message types while maintaining the same channel communication pattern.
Summary by CodeRabbit
New Features
messagespackage for standardized request and response types related to bidding and execution.AskForBidRequest,BidAcceptedRequest,BidRejectedRequest,CancelExecutionRequest,ExecutionLogsRequest,Heartbeat,RunResult,CancelResult, andComputeErrorto facilitate communication.Refactor
messagespackage instead of local types, enhancing consistency and maintainability.Tests
messagespackage, ensuring compatibility with updated request and response structures across multiple test suites.