HYPERFLEET-331: fix TSL syntax for resource selector search parameter#31
Conversation
|
Warning Rate limit exceeded@rafabene has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe PR changes labelSelectorToSearchString to emit Tree Search Language (TSL) predicates (each label -> Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labelsapproved Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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/client/client.go (1)
173-181: Optionally preallocatepartsslice for small efficiency winYou can preallocate
partsto avoid potential reallocations when building the slice, especially if selectors grow over time:- var parts []string + parts := make([]string, 0, len(labelSelector))Given the likely small map sizes this is minor, but it’s a cheap improvement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/client/client.go(1 hunks)internal/client/client_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/client/client_test.go
🔇 Additional comments (1)
internal/client/client.go (1)
162-182: TSL formatting and escaping look correctThe function and its documentation align with the described TSL requirements:
labels.prefix, single-quoted values with proper'escaping, deterministic ordering via sorting, andandconjunctions. Behavior for empty/nil selectors is also well handled.
rafabene
force-pushed
the
HYPERFLEET-331
branch
2 times, most recently
from
25048c5 to
7c2b8dd
Compare
rafabene
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/client/client.go (1)
173-177: Consider documenting the escaping behavior.The single-quote escaping by doubling (
'→'') follows SQL/TSL conventions. Consider adding a brief inline comment explaining this is the standard TSL escape sequence for single quotes, which will help future maintainers understand the intent.parts := make([]string, 0, len(labelSelector)) for k, v := range labelSelector { - // Escape single quotes in values by doubling them (SQL/TSL standard) + // Escape single quotes by doubling them ('' is the TSL escape sequence for a literal ') escapedValue := strings.ReplaceAll(v, "'", "''") parts = append(parts, fmt.Sprintf("labels.%s='%s'", k, escapedValue)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/client/client.go(1 hunks)internal/client/client_test.go(3 hunks)test/integration/integration_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/integration/integration_test.go (2)
test/integration/testcontainer.go (1)
NewHelper(122-141)internal/config/config.go (2)
SentinelConfig(24-33)LabelSelector(15-18)
🔇 Additional comments (5)
internal/client/client_test.go (2)
476-515: Comprehensive TSL syntax test coverage.The test cases appropriately cover the key TSL syntax requirements:
- Single and multiple label selectors with correct
labels.prefix- Sorted output for deterministic testing
- Edge cases for hyphens/underscores in keys and values
- Proper escaping of single quotes by doubling (
'→'')
633-636: LGTM!The integration test expectation correctly validates the TSL format with multiple labels sorted alphabetically and joined with
and.test/integration/integration_test.go (2)
207-223: LGTM - TSL syntax matching updated correctly.The server-side filtering logic now correctly matches the TSL format
labels.shard='1'instead of the old key=value format.
274-380: Well-structured integration test for TSL syntax validation.The test correctly:
- Sets up a mock server that validates the exact TSL syntax
- Configures multiple label selectors
- Validates the received search parameter matches expected TSL format
- Cleans up properly with context cancellation
One minor observation:
receivedSearchParamis written by the HTTP handler goroutine and read by the test goroutine. While the 300ms sleep provides sufficient delay in practice, consider using a channel or sync primitive for stricter correctness if this test becomes flaky.internal/client/client.go (1)
162-182: TSL conversion implementation is correct.The implementation properly:
- Prefixes keys with
labels.- Escapes single quotes in values by doubling (standard SQL/TSL escaping)
- Joins multiple conditions with
and- Sorts keys for deterministic output
The edge case concern about special characters in label keys is not valid. Kubernetes label key restrictions explicitly allow only alphanumerics, dashes, underscores, and dots (plus "/" for the optional DNS subdomain prefix). Single quotes are not permitted in Kubernetes label keys, so the current implementation—which escapes values but not keys—is correct and requires no changes.
The labelSelectorToSearchString() function was generating incorrect search parameter format that didn't match the HyperFleet API's TSL (Tree Search Language) syntax. Changes: - Prefix label keys with "labels." (e.g., labels.region) - Quote values with single quotes (e.g., 'us-east') - Use " and " instead of comma for multiple labels - Escape single quotes in values by doubling them (SQL/TSL standard) - Pre-allocate slice for better performance - Add edge case tests for special characters Before: region=us-east,env=prod After: labels.env='prod' and labels.region='us-east' This fix enables the resource selector feature to work correctly with the HyperFleet API, unblocking multi-instance deployment sharding functionality.
rafabene
force-pushed
the
HYPERFLEET-331
branch
from
7c2b8dd to
39e001a
Compare
|
I had an issue with trying with real hyperfleet-api, but the code here is already following the documented syntax. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-amarin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 09df31a
into
openshift-hyperfleet:main
2 participants
Summary
labelSelectorToSearchString()to generate correct TSL (Tree Search Language) syntax for HyperFleet APIlabels.(e.g.,labels.region)'us-east')andinstead of commaBefore:
region=us-east,env=prodAfter:
labels.env='prod' and labels.region='us-east'Test plan
labelSelectorToSearchString()TestFetchResources_WithLabelSelectorupdatedTestIntegration_TSLSyntaxMultipleLabelsaddedReferences
hyperfleet-api/README.mdandsearch_field_mapping_test.goSummary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.