-
Notifications
You must be signed in to change notification settings - Fork 219
fix: restore jqFilters #756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Updated the ApplyFilter method in the Filter interface and its implementation to return a byte slice instead of a map. This change affects multiple files, ensuring consistency in data handling across the filter package and related components.
Modified test cases in apply_test.go to correctly unmarshal the byte slice results returned by the ApplyFilter function. This ensures that the tests validate the output as expected after the recent change in return type.
Cleaned up the go.mod and go.sum files by removing several unused indirect dependencies, including docker-related packages and others that are no longer needed. This helps streamline the dependency management and reduces potential bloat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR restores the behavior of jqFilters by updating the return type of the applyFilter function from map[string]any to []byte to better support snapshot processing in Kubernetes. Key changes include updating type signatures in the filter functions, modifying the conversion logic in the kube events manager and object patch helpers, and updating the associated tests to accommodate the new data type.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
pkg/kube_events_manager/filter.go | Updated filter result handling by switching from a map to a byte slice. |
pkg/kube/object_patch/helpers.go | Modified patch conversion to decode a JSON string to an Unstructured object. |
pkg/filter/jq/apply.go | Changed ApplyFilter signature and now returns a marshalled slice. |
pkg/filter/filter.go | Updated the Filter interface to reflect the new return type. |
pkg/filter/jq/apply_test.go | Adjusted tests to unmarshal the returned JSON, with a potential issue when the filter returns multiple documents. |
Files not reviewed (1)
- go.mod: Language not supported
Modified the test cases in apply_test.go to accommodate the new return type of ApplyFilter, which now returns a slice of maps instead of a single map. This ensures that the tests accurately validate the output structure and length after the recent refactor.
Added new test cases in apply_test.go to cover various edge cases for the ApplyFilter function, including handling nil input, empty filters, invalid JSON, and empty results. This improves the robustness of the test suite and ensures comprehensive validation of the ApplyFilter behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR restores snapshot processing by reverting the jq filter’s return type back to a JSON-encoded []byte so that the processing of jqFilters in Kubernetes works as expected. Key changes include:
- Updating the applyFilter function to return JSON bytes instead of a map.
- Adjusting kube_events_manager and object_patch helpers to process JSON string values.
- Modifying tests and interface definitions to reflect the new output format.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
pkg/kube_events_manager/filter.go | Converts filter output to string and computes checksum from JSON bytes. |
pkg/kube/object_patch/helpers.go | Decodes JSON bytes into an unstructured object for patching. |
pkg/filter/jq/apply_test.go | Updates tests to unmarshal and validate JSON array outputs. |
pkg/filter/jq/apply.go | Changes applyFilter to return JSON bytes and aggregates results in a slice. |
pkg/filter/filter.go | Updates the Filter interface to match the new output type. |
Files not reviewed (1)
- go.mod: Language not supported
Comments suppressed due to low confidence (1)
pkg/filter/jq/apply.go:41
- The current implementation only appends values of type map[string]any to the results and silently ignores other types. If the jq filter might return non-map values, consider handling these cases or explicitly documenting that only object results are expected.
for { v, ok := iter.Next()
Modified the ApplyFilter function to return a slice of any instead of a slice of maps. Corresponding changes were made in the test cases to reflect this new return type, ensuring accurate validation of the output structure and behavior in various scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR restores the previous behavior of jqFilters by changing the return type of the applyFilter function from map[string]any to []byte, ensuring that snapshot processing in Kubernetes is handled correctly. Key changes include:
- Updating the signature and implementation of the applyFilter function in pkg/kube_events_manager/filter.go and pkg/filter/jq/apply.go.
- Adjusting the object patch helper to decode filter results properly.
- Revising tests in pkg/filter/jq/apply_test.go to validate the updated JSON output.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
pkg/kube_events_manager/filter.go | Modified applyFilter to work with byte slices for filter results and checksum. |
pkg/kube/object_patch/helpers.go | Changed patch application to decode from JSON bytes into an Unstructured object. |
pkg/filter/jq/apply_test.go | Updated test expectations and added new tests covering nil, empty and invalid inputs. |
pkg/filter/jq/apply.go | Refactored ApplyFilter to accumulate results in an array and return marshaled JSON. |
pkg/filter/filter.go | Updated Filter interface to match the new byte slice return type. |
Files not reviewed (1)
- go.mod: Language not supported
Comments suppressed due to low confidence (1)
pkg/filter/jq/apply.go:30
- Returning filter results as a JSON array may break compatibility if downstream consumers expect an object; verify that all consumers are updated to handle this array format.
result := make([]any, 0)
Refactored test cases in apply_test.go to eliminate redundant checks for the length of resultMap. The tests now directly validate the expected output structure, ensuring clarity and conciseness in the test assertions.
…ectl installation Added a new stage to the Dockerfile to include a prebuilt version of libjq. Updated the RUN command for installing kubectl to improve readability and maintainability. This change enhances the overall build process and ensures the necessary dependencies are included in the final image.
Updated the RUN command in the Dockerfile to enhance readability by using line continuation. This change maintains the functionality while making the installation process for kubectl clearer and more maintainable.
How I tested: root@dev-master-0:~/src/module# kubectl -n d8-system exec deployments/deckhouse -- deckhouse-controller version
deckhouse v1.70.0-pr13285+d5980cc (addon-operator v1.7.1-0.20250429144936-56c46a1eb2af, shell-operator v1.7.2-0.20250429144844-5f1f44619e7a, Golang go1.23.8)
root@dev-master-0:~/src/module# kubectl get modules | grep stronghold
stronghold 900 deckhouse Ready True True
root@dev-master-0:~/src/module# kubectl -n d8-system exec deployments/deckhouse -- deckhouse-controller module snapshots stronghold -ojson | jq . | less
{
"v1.15.2/hooks/certificate.py": null,
"v1.15.2/hooks/copy_custom_certificate.py": {
"custom_certificates": {
"operations": {
"sinceLastExecution": {
"count": 0,
"added": 0,
"deleted": 0,
"modified": 0,
"cleaned": 0
},
"sinceStart": {
"count": 14,
"added": 14,
"deleted": 0,
"modified": 0,
"cleaned": 0
}
},
"snapshot": [
{
"filterResult": {
"data": {
"ca.crt": "LS0tLS1CRUdJTiBDR
... |
I saved the snapshots that kubectl outputs from the stronghold module to a json file. Then I switched the dev cluster to the main branch and repeated the generation with saving the result to another file: root@dev-master-0:~/src/module# diff ~/stronghold-snapshots*
10c10
< "modified": 0,
---
> "modified": 42,
17c17
< "modified": 0,
---
> "modified": 42,
960c960
< "modified": 0,
---
> "modified": 4,
967c967
< "modified": 0,
---
> "modified": 4, In other words, the results are identical |
Signed-off-by: Pavel Okhlopkov <[email protected]>
Overview
After changing the jq dependencies to the itchyny/gojq library, we stopped processing snapshots correctly in kubernetes.
What this PR does / why we need it
The return value of the applyFilter function has been changed from map[string]any to [] byte, which is close to what it was
Special notes for your reviewer
Checking changes in the dev cluster