fix(tfpluginsdk): fix ignore changes path for converted singleton lists#633
Open
castorw wants to merge 2 commits intocrossplane:mainfrom
Open
fix(tfpluginsdk): fix ignore changes path for converted singleton lists#633castorw wants to merge 2 commits intocrossplane:mainfrom
castorw wants to merge 2 commits intocrossplane:mainfrom
Conversation
castorw
requested review from
erhancagirici,
sergenyalcin and
ulucinar
as code owners
Signed-off-by: Lubomir Kaplan <castor@castor.sk>
castorw
force-pushed
the
fix-singleton-ignore-changes
branch
from
256ed6a to
2781a2c
Compare
📝 WalkthroughWalkthroughThe change extends Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/external_tfpluginsdk.go (1)
391-403:⚠️ Potential issue | 🟠 MajorClear the singleton-list container length key too.
After Line 349 converts
scaling_config.desired_sizetoscaling_config.0.desired_size, this loop computesscaling_config.0.#; the Plugin SDK list length diff isscaling_config.#. If that parent length key remains, the diff can still look non-empty and trigger the update loop this PR is trying to prevent.🐛 Suggested addition for converted singleton prefixes
for _, lengthSymbol := range []string{"%", "#"} { keyComponents[len(keyComponents)-1] = lengthSymbol lengthKey := strings.Join(keyComponents, ".") delete(instanceDiff.Attributes, lengthKey) } + + for _, p := range config.TFListConversionPaths() { + p = normalizeTFListConversionPath(p) + convertedPrefix := fmt.Sprintf("%s.0.", p) + if !strings.HasPrefix(keyToIgnore, convertedPrefix) { + continue + } + delete(instanceDiff.Attributes, fmt.Sprintf("%s.#", p)) + delete(instanceDiff.Attributes, fmt.Sprintf("%s.%%", p)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/external_tfpluginsdk.go` around lines 391 - 403, The loop over initProviderExclusiveParamKeys currently deletes length keys like "scaling_config.0.#" but misses the parent list length "scaling_config.#", so keep causing spurious diffs; update the loop that builds lengthKey (in the same block operating on instanceDiff.Attributes) to also compute and delete the corresponding parent-length key when a singleton index was inserted — specifically, after creating lengthKey from keyComponents, if the component immediately before the length symbol is "0", construct parentLengthKey by removing that ".0" segment (e.g., turn "scaling_config.0.#" into "scaling_config.#") and call delete(instanceDiff.Attributes, parentLengthKey) as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/external_tfpluginsdk.go`:
- Around line 342-358: Normalize TFListConversionPaths() entries by replacing
"[0]" and "[*]" with "0" and trimming/ensuring dot separators, sort the
normalized paths by descending length so longer prefixes match first, then
iterate over those normalizedPaths in the loop that builds
initProviderExclusiveParamKeysConverted (use matchPrefix := fmt.Sprintf("%s.",
normalizedPath)) and on first match replace the prefix with fmt.Sprintf("%s0.",
matchPrefix) and break the inner loop to avoid duplicate partial conversions;
add table-driven unit tests exercising patterns with "[0]", "[*]", nested and
overlapping paths to ensure one deterministic converted key per input.
---
Outside diff comments:
In `@pkg/controller/external_tfpluginsdk.go`:
- Around line 391-403: The loop over initProviderExclusiveParamKeys currently
deletes length keys like "scaling_config.0.#" but misses the parent list length
"scaling_config.#", so keep causing spurious diffs; update the loop that builds
lengthKey (in the same block operating on instanceDiff.Attributes) to also
compute and delete the corresponding parent-length key when a singleton index
was inserted — specifically, after creating lengthKey from keyComponents, if the
component immediately before the length symbol is "0", construct parentLengthKey
by removing that ".0" segment (e.g., turn "scaling_config.0.#" into
"scaling_config.#") and call delete(instanceDiff.Attributes, parentLengthKey) as
well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce6e0bbf-f5c7-4f66-88df-c0cfaba916c6
📒 Files selected for processing (1)
pkg/controller/external_tfpluginsdk.go
Comment on lines
+342
to
+358
| /* process singleton list expansions */ | ||
| initProviderExclusiveParamKeysConverted := []string{} | ||
| for _, key := range initProviderExclusiveParamKeys { | ||
| matched := false | ||
| for _, p := range config.TFListConversionPaths() { | ||
| matchPrefix := fmt.Sprintf("%s.", p) | ||
| if strings.HasPrefix(key, matchPrefix) { | ||
| initProviderExclusiveParamKeysConverted = append(initProviderExclusiveParamKeysConverted, strings.Replace(key, matchPrefix, fmt.Sprintf("%s0.", matchPrefix), 1)) | ||
| matched = true | ||
| continue | ||
| } | ||
| } | ||
| if !matched { | ||
| initProviderExclusiveParamKeysConverted = append(initProviderExclusiveParamKeysConverted, key) | ||
| } | ||
| } | ||
| initProviderExclusiveParamKeys = initProviderExclusiveParamKeysConverted |
There was a problem hiding this comment.
Normalize and apply singleton-list conversions deterministically.
Thanks for fixing the simple scaling_config case. This loop still compares raw TFListConversionPaths() values directly, but those paths may contain [0]/[*]; overlapping singleton paths can also append duplicate partial keys because continue stays inside the inner loop. Please normalize paths and produce one fully converted key per input, ideally applying longer prefixes first.
🐛 Suggested direction
import (
"context"
"fmt"
+ "sort"
"strings"
"time"
@@
+func normalizeTFListConversionPath(p string) string {
+ p = strings.ReplaceAll(p, "[*]", "")
+ return strings.ReplaceAll(p, "[0]", "")
+}
+
func filterInitExclusiveDiffs(config *config.Resource, tr resource.Terraformed, instanceDiff *tf.InstanceDiff) error { //nolint:gocyclo
@@
/* process singleton list expansions */
- initProviderExclusiveParamKeysConverted := []string{}
+ conversionPaths := config.TFListConversionPaths()
+ sort.Slice(conversionPaths, func(i, j int) bool {
+ return len(normalizeTFListConversionPath(conversionPaths[i])) > len(normalizeTFListConversionPath(conversionPaths[j]))
+ })
+
+ initProviderExclusiveParamKeysConverted := make([]string, 0, len(initProviderExclusiveParamKeys))
for _, key := range initProviderExclusiveParamKeys {
- matched := false
- for _, p := range config.TFListConversionPaths() {
+ convertedKey := key
+ for _, p := range conversionPaths {
+ p = normalizeTFListConversionPath(p)
matchPrefix := fmt.Sprintf("%s.", p)
- if strings.HasPrefix(key, matchPrefix) {
- initProviderExclusiveParamKeysConverted = append(initProviderExclusiveParamKeysConverted, strings.Replace(key, matchPrefix, fmt.Sprintf("%s0.", matchPrefix), 1))
- matched = true
- continue
+ if strings.HasPrefix(convertedKey, matchPrefix) {
+ convertedKey = strings.Replace(convertedKey, matchPrefix, fmt.Sprintf("%s0.", matchPrefix), 1)
}
}
- if !matched {
- initProviderExclusiveParamKeysConverted = append(initProviderExclusiveParamKeysConverted, key)
- }
+ initProviderExclusiveParamKeysConverted = append(initProviderExclusiveParamKeysConverted, convertedKey)
}Please add table-driven coverage for [0], [*], and nested/overlapping conversion paths. As per coding guidelines, All Upjet code must be covered by tests. Do not use Ginkgo tests or third party testing libraries; use Go's standard testing approach. Upjet encourages the use of table driven unit tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/external_tfpluginsdk.go` around lines 342 - 358, Normalize
TFListConversionPaths() entries by replacing "[0]" and "[*]" with "0" and
trimming/ensuring dot separators, sort the normalized paths by descending length
so longer prefixes match first, then iterate over those normalizedPaths in the
loop that builds initProviderExclusiveParamKeysConverted (use matchPrefix :=
fmt.Sprintf("%s.", normalizedPath)) and on first match replace the prefix with
fmt.Sprintf("%s0.", matchPrefix) and break the inner loop to avoid duplicate
partial conversions; add table-driven unit tests exercising patterns with "[0]",
"[*]", nested and overlapping paths to ensure one deterministic converted key
per input.
Signed-off-by: Lubomir Kaplan <castor@castor.sk>
castorw
changed the title
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
Fixes crossplane-contrib/provider-upjet-aws#2044
Fixes crossplane-contrib/provider-upjet-aws#1907
This PR updates the process of
ignore_changespaths computation which did not handle converted singleton lists properly. Resources represented as objects in CRDs and blocks/lists were added improperly to theignore_changeslist. Example (and apparently the most usual use case) foreks.aws.m.upbound.io/v1beta1/NodeGroup:scaling_config.0.desired_sizeis the expected output and correct value,scaling_config.desired_sizeis the invalid output produced by the plugin.Here we have added the missing
.0path segment to circumvent this issue.Fixing this should resolve the underlying issue for all resource types but primarily fixing issue preventing use of automated scaling (eg.
cluster-autoscaler) for EKS clusters.I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
Steps to reproduce the issue are detailed in my original issue crossplane-contrib/provider-upjet-aws#2044. Here's a guideline for testing:
eks.aws.m.upbound.io/v1beta1/Cluster,eks.aws.m.upbound.io/v1beta1/NodeGroupwith the following options:ignore_changes(scaling_config.desired_sizevsscaling_config.0.desired_size).