Fix multivalue in match expression #15
Conversation
WalkthroughLabelSelector serialization/deserialization was reworked: fromProto now builds structured Terraform Object entries for MatchExpressions (key, operator, values) and MatchLabels empty collections become null. Tests added/extended to cover multiple MatchExpressions, multi-value lists, empty collections, and round-trip integrity. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Tests
participant P as protobuf LabelSelector
participant M as WorkloadPolicyTarget.fromProto
note right of M `#DDEBF7`: Change — build structured Object entries
T->>P: construct LabelSelector proto (labels + matchExpressions)
T->>M: call fromProto(proto)
M->>M: if MatchLabels empty -> set null\nelse -> map to Terraform map
M->>M: for each proto.matchExpression\n map enum -> string operator\n convert values -> Terraform string list\n build Object { key, operator, values }
M->>M: collect objects -> List(ObjectType) or null if empty
M-->>T: return model with structured MatchExpressions and MatchLabels
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (1)
internal/provider/workload_policy_target_test.go (1)
42-42: Update element type to match the schema structure.This line creates an empty list with
StringTypeelements, but based on the fix at line 648 and the schema definition,MatchExpressionsshould useObjectTypewithMatchExpressionattributes for consistency.Apply this diff:
- MatchExpressions: types.ListValueMust(types.StringType, []attr.Value{}), // Simplified for testing + MatchExpressions: types.ListValueMust(types.ObjectType{AttrTypes: MatchExpression{}.AttrTypes()}, []attr.Value{}), // Simplified for testing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/provider/node_policy_test.go(1 hunks)internal/provider/workload_policy_target.go(1 hunks)internal/provider/workload_policy_target_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/provider/workload_policy_target_test.go (1)
internal/provider/workload_policy_target.go (1)
LabelSelector(58-61)
internal/provider/node_policy_test.go (2)
internal/provider/workload_policy_target.go (1)
LabelSelector(58-61)internal/gen/api/v1/common.pb.go (3)
LabelSelector(2966-2981)LabelSelector(2996-2996)LabelSelector(3011-3013)
🔇 Additional comments (3)
internal/provider/workload_policy_target_test.go (1)
126-258: Excellent test coverage for multiple match expressions.The test comprehensively validates the fix for multivalue match expressions, covering:
- Multiple expressions (In, NotIn, Exists)
- Multiple values within expressions (3 values for In, 2 for NotIn)
- Correct operator enum mappings (IN=1, NOT_IN=2, EXISTS=3)
- Empty values for Exists operator
internal/provider/node_policy_test.go (1)
299-434: Excellent test coverage for node policy match expressions.The test comprehensively validates multivalue match expressions for node policies, covering:
- Multiple expressions (Gt, In, NotIn)
- Multiple values within expressions (1 value for Gt, 3 for In, 2 for NotIn)
- Correct operator enum mappings (GT=5, IN=1, NOT_IN=2)
- Node-specific use cases (instanceGenerations, instanceFamily, zone)
internal/provider/workload_policy_target.go (1)
648-648: All verification checks passed—code change is correct.The review comment is accurate:
- ✓
fromElementListexists and correctly converts protoLabelSelectorRequirementvalues to Terraform object values usingtypes.ObjectValueFrom- ✓ Schema definition (lines 94-118) defines
match_expressionsasListNestedAttributewith nested objects- ✓
MatchExpression.AttrTypes()provides the correct object schema (key, operator, values)- ✓
toProtomethod (lines 533-552) expects and properly extracts these object attributes- ✓ The
ObjectTypechange at line 648 correctly aligns with the schema and proto conversion flowNo concerns remain. The implementation is sound.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/provider/workload_policy_target.go (1)
648-684: Add default case to operator switch for robustness.The operator switch statement lacks a default case. If an unknown or future operator enum value is received from the proto,
operatorStrwill remain empty, potentially causing silent failures or confusing behavior in Terraform state.Consider adding a default case:
switch expr.Operator { case apiv1.LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_IN: operatorStr = "In" case apiv1.LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_NOT_IN: operatorStr = "NotIn" case apiv1.LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_EXISTS: operatorStr = "Exists" case apiv1.LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_DOES_NOT_EXIST: operatorStr = "DoesNotExist" case apiv1.LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_GT: operatorStr = "Gt" case apiv1.LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_LT: operatorStr = "Lt" + default: + operatorStr = "Unknown" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/provider/workload_policy_target.go(1 hunks)internal/provider/workload_policy_target_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/provider/workload_policy_target_test.go (1)
internal/provider/workload_policy_target.go (1)
LabelSelector(58-61)
internal/provider/workload_policy_target.go (1)
internal/gen/api/v1/common.pb.go (6)
LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_IN(208-208)LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_NOT_IN(209-209)LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_EXISTS(210-210)LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_DOES_NOT_EXIST(211-211)LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_GT(212-212)LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_LT(213-213)
🪛 GitHub Actions: Tests
internal/provider/workload_policy_target_test.go
[error] 387-387: type assertion must be checked (forcetypeassert)
🪛 GitHub Check: Build
internal/provider/workload_policy_target_test.go
[failure] 388-388:
type assertion must be checked (forcetypeassert)
[failure] 387-387:
type assertion must be checked (forcetypeassert)
🔇 Additional comments (1)
internal/provider/workload_policy_target_test.go (1)
126-258: Excellent test coverage for multiple match expressions.This test thoroughly validates the toProto conversion with multiple match expressions and multiple values, covering different operators (In, NotIn, Exists) and edge cases like empty values for the Exists operator.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/provider/workload_policy_target.go(1 hunks)internal/provider/workload_policy_target_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/provider/workload_policy_target.go (1)
internal/gen/api/v1/common.pb.go (6)
LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_IN(208-208)LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_NOT_IN(209-209)LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_EXISTS(210-210)LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_DOES_NOT_EXIST(211-211)LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_GT(212-212)LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_LT(213-213)
internal/provider/workload_policy_target_test.go (1)
internal/provider/workload_policy_target.go (1)
LabelSelector(58-61)
| if len(selector.MatchLabels) == 0 { | ||
| m.MatchLabels = types.MapNull(types.StringType) | ||
| } else { | ||
| m.MatchLabels = types.MapValueMust(types.StringType, fromStringMap(selector.MatchLabels)) | ||
| } | ||
|
|
||
| // Manually convert match expressions from proto to Terraform types | ||
| var matchExpressions []attr.Value | ||
| for _, expr := range selector.MatchExpressions { | ||
| // Convert operator enum to string | ||
| var operatorStr string | ||
| switch expr.Operator { | ||
| case apiv1.LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_IN: | ||
| operatorStr = "In" | ||
| case apiv1.LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_NOT_IN: | ||
| operatorStr = "NotIn" | ||
| case apiv1.LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_EXISTS: | ||
| operatorStr = "Exists" | ||
| case apiv1.LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_DOES_NOT_EXIST: | ||
| operatorStr = "DoesNotExist" | ||
| case apiv1.LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_GT: | ||
| operatorStr = "Gt" | ||
| case apiv1.LabelSelectorOperator_LABEL_SELECTOR_OPERATOR_LT: | ||
| operatorStr = "Lt" | ||
| } | ||
|
|
||
| // Convert values to Terraform list | ||
| values := types.ListValueMust(types.StringType, fromStringList(expr.Values)) | ||
|
|
||
| // Build the match expression object | ||
| matchExpr := types.ObjectValueMust( | ||
| MatchExpression{}.AttrTypes(), | ||
| map[string]attr.Value{ | ||
| "key": types.StringValue(expr.Key), | ||
| "operator": types.StringValue(operatorStr), | ||
| "values": values, | ||
| }, | ||
| ) | ||
| matchExpressions = append(matchExpressions, matchExpr) | ||
| } | ||
|
|
||
| // Handle match_expressions: if empty, set to null instead of empty list | ||
| if len(matchExpressions) == 0 { | ||
| m.MatchExpressions = types.ListNull(types.ObjectType{AttrTypes: MatchExpression{}.AttrTypes()}) | ||
| } else { | ||
| m.MatchExpressions = types.ListValueMust(types.ObjectType{AttrTypes: MatchExpression{}.AttrTypes()}, matchExpressions) | ||
| } |
There was a problem hiding this comment.
Nil selector leaves stale state
When selector is nil, we return before touching the new null-handling logic. Because m = nil only rebinds the local copy of the pointer, any existing selector struct (and its MatchLabels/MatchExpressions) survives unchanged. This means removing a selector server-side leaves the Terraform state full of the previous values. Please clear the fields instead of bailing early, for example:
- if selector == nil {
- m = nil
- return
- }
+ if selector == nil {
+ if m == nil {
+ return
+ }
+ m.MatchLabels = types.MapNull(types.StringType)
+ m.MatchExpressions = types.ListNull(types.ObjectType{AttrTypes: MatchExpression{}.AttrTypes()})
+ return
+ }This way the state reflects the absence of a selector and we avoid perpetual drift.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
internal/provider/workload_policy_target.go around lines 649 to 695: if selector
is nil do not return early; instead explicitly clear the selector fields on m so
Terraform state is updated — set m.MatchLabels = types.MapNull(types.StringType)
and set m.MatchExpressions = types.ListNull(types.ObjectType{AttrTypes:
MatchExpression{}.AttrTypes()}) (then return). This replaces the early bail so
existing state is nulled when the server-side selector is removed.
Summary by CodeRabbit
Tests
Bug Fixes