feat: add node selector to intance #594
Conversation
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
WalkthroughThe changes introduce several enhancements across multiple files related to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Build
participant Execution
participant K8s
User->>Build: SetNodeSelector(nodeSelector)
Build->>Build: Check State
alt State Valid
Build->>Build: Update nodeSelector
Build->>Execution: Prepare ReplicaSet Config
Execution->>K8s: Include NodeSelector in PodConfig
K8s->>K8s: Create Pod with NodeSelector
else State Invalid
Build->>User: Return ErrSettingNodeSelectorNotAllowed
end
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: 0
🧹 Outside diff range and nitpick comments (2)
pkg/instance/build.go (1)
163-170: Consider adding validation for nodeSelector contents.While the state validation is good, consider adding validation for the nodeSelector map contents to ensure:
- The map is not empty
- Keys follow Kubernetes label key format
- Values follow Kubernetes label value format
func (b *build) SetNodeSelector(nodeSelector map[string]string) error { if !b.instance.IsInState(StatePreparing, StateCommitted) { return ErrSettingNodeSelectorNotAllowed.WithParams(b.instance.state.String()) } + + if len(nodeSelector) == 0 { + return fmt.Errorf("nodeSelector cannot be empty") + } + + // Validate keys and values follow k8s label format + for k, v := range nodeSelector { + if !isValidLabelKey(k) || !isValidLabelValue(v) { + return fmt.Errorf("invalid nodeSelector key/value: %s=%s", k, v) + } + } b.nodeSelector = nodeSelector return nil }pkg/k8s/pod.go (1)
65-65: Consider adding validation for node selector keys and values.The NodeSelector field is correctly typed as
map[string]stringmatching Kubernetes requirements. However, consider adding validation to ensure:
- Keys follow Kubernetes label key format (prefix/name)
- Values follow Kubernetes label value format (63 characters max, alphanumeric, '-', '_', '.')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
pkg/instance/build.go(2 hunks)pkg/instance/errors.go(1 hunks)pkg/instance/execution.go(1 hunks)pkg/k8s/pod.go(2 hunks)pkg/knuu/knuu.go(1 hunks)
🔇 Additional comments (5)
pkg/knuu/knuu.go (1)
112-113: LGTM: Good practice to reset signal handlers.
The addition of signal handler reset after cleanup is a good practice as it allows other signal handlers to run and prevents potential handler conflicts.
Let's verify there are no other signal handlers that might be affected:
✅ Verification successful
Signal handling implementation is correctly isolated and reset
The verification confirms that there is only one signal handler registration in the codebase using signal.Notify(), which is in the same file (pkg/knuu/knuu.go) where the reset is performed. The reset is correctly placed after the handler has served its purpose, ensuring clean signal handling management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other signal handler registrations in the codebase
# Test: Search for signal.Notify calls to ensure we're not interfering with other handlers
rg -A 2 "signal\.Notify\("
Length of output: 169
pkg/instance/build.go (1)
28-28: LGTM: Well-typed nodeSelector field.
The nodeSelector field is correctly typed as map[string]string to match Kubernetes node selector requirements.
pkg/instance/execution.go (1)
420-420: LGTM: Proper integration of nodeSelector.
The nodeSelector is correctly passed from the build configuration to the PodConfig, maintaining the proper configuration chain.
pkg/k8s/pod.go (1)
627-627: LGTM: NodeSelector correctly integrated into pod spec.
The NodeSelector is properly assigned from PodConfig to the Kubernetes PodSpec.
pkg/instance/errors.go (1)
117-117: LGTM: Error definition follows established patterns.
The error variable is well-defined and consistent with other state-restricted setting errors in the codebase.
Overview
Summary by CodeRabbit
New Features
nodeSelectorfield for enhanced build configuration.NodeSelectorfield to the pod configuration, allowing for specific node selection during deployment.Bug Fixes
Documentation
Chores