Gcp Labels (tags) Datasource #38
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new lint target to the Makefile that runs GolangCI-Lint through Docker using the Changes
✨ Finishing Touches
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. 🪧 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 (
|
Signed-off-by: Benjamin Smith <[email protected]>
Signed-off-by: Benjamin Smith <[email protected]>
Signed-off-by: Benjamin Smith <[email protected]>
Signed-off-by: Benjamin Smith <[email protected]>
Signed-off-by: Benjamin Smith <[email protected]>
Signed-off-by: Benjamin Smith <[email protected]>
Signed-off-by: Benjamin Smith <[email protected]>
Signed-off-by: Benjamin Smith <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
docs/data-sources/gcp_labels.md (1)
1-30: Documentation format and content look goodThe documentation for the new GCP Labels data source follows Terraform's standard documentation format and provides clear descriptions for all attributes.
Minor grammar issue in line 20: Remove the comma after "tag" since the "applies" clause is essential to the sentence's meaning:
- `replacement_map` (Map of String) Map of strings to replace in the tag, applies to both key and value. The key is the string to replace, and the value is the string to replace it with. + `replacement_map` (Map of String) Map of strings to replace in the tag applies to both key and value. The key is the string to replace, and the value is the string to replace it with.🧰 Tools
🪛 LanguageTool
[formatting] ~20-~20: If the ‘applies’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...tring) Map of strings to replace in the tag, applies to both key and value. The key is the s...(COMMA_AFTER_PREPOSITION_PHRASES)
internal/provider/gcp_labels_data_source_test.go (1)
34-60: Fix indentation inconsistencyThe test configuration has an inconsistent indentation in the values map.
values = { "Namespace" = "cloudposse" "Tenant" = "core" "Stage" = "prod" "Name" = "example" - "ComponentPath" = "foo/bar/baz" + "ComponentPath" = "foo/bar/baz" }internal/provider/gcp_labels_data_source.go (2)
79-83: Consider clarifying the documentation forreplacement_map.The
replacement_mapattribute is powerful but might cause confusion on how multiple replacements are applied. Including a short note or example in the schema’s Markdown description to explain how patterns will be replaced would improve clarity for end users.
88-88: Clarify the reason for ignoringgocriticwarnings.The
//nolint:gocriticcomment is present without an explanatory note. Consider adding a rationale for it or removing it if it’s no longer needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(1 hunks)docs/data-sources/gcp_labels.md(1 hunks)internal/provider/gcp_labels_data_source.go(1 hunks)internal/provider/gcp_labels_data_source_test.go(1 hunks)internal/provider/provider.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
internal/provider/gcp_labels_data_source.go (1)
internal/provider/tags_data_source.go (2)
TagsDataSource(27-29)TagsDataSourceModel(32-39)
🪛 LanguageTool
docs/data-sources/gcp_labels.md
[formatting] ~20-~20: If the ‘applies’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...tring) Map of strings to replace in the tag, applies to both key and value. The key is the s...
(COMMA_AFTER_PREPOSITION_PHRASES)
🔇 Additional comments (4)
Makefile (1)
40-41: Excellent addition of lint target using DockerThe implementation of a Docker-based
linttarget ensures consistent linting across different environments and makes it easy for developers to run linting without installing golangci-lint locally. The use of the--fixflag is particularly helpful as it will automatically address simple issues.internal/provider/provider.go (1)
246-246: LGTM: GCP Labels data source properly registeredThe new GCP Labels data source is correctly registered in the provider's DataSources method, following the established pattern used for other data sources.
internal/provider/gcp_labels_data_source_test.go (1)
9-32: Well-structured acceptance testThe test thoroughly validates the GCP Labels data source functionality, including the string replacement feature and proper formatting of tags as a list.
internal/provider/gcp_labels_data_source.go (1)
19-21: Looks good regarding Terraform DataSource interface compliance.The
var _ datasource.DataSource = &GcpLabelsDataSource{}block confirms thatGcpLabelsDataSourceproperly satisfies the Terraform DataSource interface.
| for tagKey, tagValue := range values { | ||
| for old, newString := range replacementMap { | ||
| newTagKey := strings.ReplaceAll(tagKey, old, newString) | ||
| newTagValue := strings.ReplaceAll(tagValue, old, newString) | ||
| replacedValues[newTagKey] = newTagValue | ||
| } |
There was a problem hiding this comment.
Possible collisions and repeated transformations in runReplaceOnTags.
Because the code executes replacements in a nested loop for each (old, newString) pair, it re-binds the key multiple times for a single original key. This can cause collisions or values to be overwritten if multiple old→new transformations collide into the same key. Consider applying all replacements in a single pass, for example:
-for tagKey, tagValue := range values {
- for old, newString := range replacementMap {
- newTagKey := strings.ReplaceAll(tagKey, old, newString)
- newTagValue := strings.ReplaceAll(tagValue, old, newString)
- replacedValues[newTagKey] = newTagValue
- }
+for originalKey, originalValue := range values {
+ newKey := originalKey
+ newValue := originalValue
+ // Apply all old→new transformations once for each key-value pair
+ for old, newString := range replacementMap {
+ newKey = strings.ReplaceAll(newKey, old, newString)
+ newValue = strings.ReplaceAll(newValue, old, newString)
+ }
+ replacedValues[newKey] = newValue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for tagKey, tagValue := range values { | |
| for old, newString := range replacementMap { | |
| newTagKey := strings.ReplaceAll(tagKey, old, newString) | |
| newTagValue := strings.ReplaceAll(tagValue, old, newString) | |
| replacedValues[newTagKey] = newTagValue | |
| } | |
| for originalKey, originalValue := range values { | |
| newKey := originalKey | |
| newValue := originalValue | |
| // Apply all old→new transformations once for each key-value pair | |
| for old, newString := range replacementMap { | |
| newKey = strings.ReplaceAll(newKey, old, newString) | |
| newValue = strings.ReplaceAll(newValue, old, newString) | |
| } | |
| replacedValues[newKey] = newValue | |
| } |
There was a problem hiding this comment.
This is a good suggestion from @CodeRabbit
There was a problem hiding this comment.
@mcalhoun Thank you for confirming! I noticed that the same issue also exists in the runReplaceOnTagsList method which has a similar nested loop structure. Let me implement the fix for both methods.
flowchart TB
A["Fix Replacement Methods"]
A --o B["Fix runReplaceOnTags method"]
A --o C["Fix runReplaceOnTagsList method"]
B --o D["Change nested loop structure to avoid collisions"]
C --o E["Change nested loop structure to avoid collisions"]
🧬 Code Definitions Used
Relevant Code Snippets
From internal/provider/constants.go
Line 19-19:
ValidCases = []string{CaseNone, CaseCamel, CaseLower, CaseSnake, CaseTitle, CaseUpper}- Description: This snippet defines a slice of valid string cases that can be used for tag key and value casing in the
GcpLabelsDataSource. It includes various casing options such as camel case, lower case, and others.
From internal/model/provider_config.go
Line 19-28:
ProviderConfig struct {
delimiter string
enabled bool
properties []Property
propertyOrder []string
replaceCharsRegex string
tagsKeyCase cases.Case
tagsValueCase cases.Case
values map[string]string
}- Description: This struct represents the configuration for the provider, including fields for delimiter, enabled status, properties, and the cases for tag keys and values. The
tagsKeyCaseandtagsValueCasefields are of typecases.Case, which likely relates to the valid cases defined earlier.
From internal/framework/data_conversion_helpers.go
Line 11-21:
func FromFrameworkMap[T interface{}](ctx context.Context, m types.Map) (map[string]T, diag.Diagnostics) {
localValues := make(map[string]T, len(m.Elements()))
if !m.IsNull() {
diag := m.ElementsAs(ctx, &localValues, false)
if diag.HasError() {
return nil, diag
}
}
return localValues, nil
}- Description: This function converts a
types.Mapto a native Go map. It handles potential errors during the conversion and returns the converted map along with any diagnostics.
From internal/provider/tags_data_source.go
Line 27-29:
TagsDataSource struct {
providerData *model.ProviderData
}- Description: This struct serves as a base for data sources that handle tags, containing a reference to
ProviderData, which likely includes configuration and state information for the provider.
Line 32-39:
TagsDataSourceModel struct {
Id types.String `tfsdk:"id"`
Values types.Map `tfsdk:"values"`
Tags types.Map `tfsdk:"tags"`
TagsKeyCase types.String `tfsdk:"tags_key_case"`
TagsValueCase types.String `tfsdk:"tags_value_case"`
TagsAsList types.List `tfsdk:"tags_as_list"`
}- Description: This struct models the data for the tags data source, including fields for ID, values, tags, and the casing options for keys and values. It is essential for managing the state and configuration of tags in the GCP labels data source.
From pkg/map/hash.go
Line 10-30:
func HashMap(m interface{}) string {
switch v := m.(type) {
case map[string]interface{}:
keys := make([]string, 0, len(v))
for k := range v {
keys = append(keys, k)
}
sort.Strings(keys)
h := sha256.New()
for _, k := range keys {
h.Write([]byte(k))
h.Write([]byte(HashMap(v[k])))
}
return hex.EncodeToString(h.Sum(nil))
default:
// For primitive types, convert to JSON and hash
b, _ := json.Marshal(v)
hashed := sha256.Sum256(b)
return hex.EncodeToString(hashed[:])
}
}- Description: This function computes a SHA-256 hash of a given map. It handles both maps and primitive types, ensuring that the hash is consistent and can be used for identifying unique sets of tags.
These snippets provide context on the configuration, data structures, and utility functions relevant to the GcpLabelsDataSource implementation.
Code Analysis Tools
Tool Analysis Results
CodeRabbit ran static analysis tools on your files and found the following:
🪛 LanguageTool
docs/data-sources/gcp_labels.md
[formatting] ~20-~20: If the ‘applies’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...tring) Map of strings to replace in the tag, applies to both key and value. The key is the s...
(COMMA_AFTER_PREPOSITION_PHRASES)
Files with Issues
docs/data-sources/gcp_labels.md
🪛 LanguageTool
[formatting] ~20-~20: If the ‘applies’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...tring) Map of strings to replace in the tag, applies to both key and value. The key is the s...
(COMMA_AFTER_PREPOSITION_PHRASES)
Next Steps
Based on these results, you can:
- Fix the issues identified by the tools
- Ask me to suggest specific fixes for these issues
- Proceed with planning or implementing changes while accounting for these issues
There was a problem hiding this comment.
Tip
The multi-step agents feature is in experimental mode.
A summary of the edits CodeRabbit can apply:
-
We are going to update the runReplaceOnTags method in internal/provider/gcp_labels_data_source.go to avoid collisions by applying all old→new transformations in a single pass for each key-value pair.
-
We are going to update the runReplaceOnTagsList method in internal/provider/gcp_labels_data_source.go to avoid collisions by processing each tag map in a single pass through replacementMap.
A summary of the context that CodeRabbit is considering across the codebase:
-
The updated implementations avoid multiple rebindings within nested loops by transforming each key-value pair only once using the complete set of replacements from ReplacementMap. This prevents potential collisions and repeated transformations.
-
✅ Create PR with these edits
-
📋 Get copyable edits
-
📌 Create commit in current branch
-
🔄 Try a different approach
Code Analysis Tools
Tool Analysis Results
CodeRabbit ran static analysis tools on your files and found the following:
🪛 LanguageTool
docs/data-sources/gcp_labels.md
[formatting] ~20-~20: If the ‘applies’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...tring) Map of strings to replace in the tag, applies to both key and value. The key is the s...
(COMMA_AFTER_PREPOSITION_PHRASES)
Files with Issues
docs/data-sources/gcp_labels.md
🪛 LanguageTool
[formatting] ~20-~20: If the ‘applies’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...tring) Map of strings to replace in the tag, applies to both key and value. The key is the s...
(COMMA_AFTER_PREPOSITION_PHRASES)
Next Steps
Based on these results, you can:
- Fix the issues identified by the tools
- Ask me to suggest specific fixes for these issues
- Proceed with planning or implementing changes while accounting for these issues
| if !config.ReplacementMap.IsNull() && !config.ReplacementMap.IsUnknown() { | ||
| replacementMap, _ := framework.FromFrameworkMap[string](ctx, config.ReplacementMap) | ||
|
|
||
| for _, TagKV := range values { | ||
| for old, newString := range replacementMap { | ||
| TagKV["Key"] = strings.ReplaceAll(TagKV["Key"], old, newString) | ||
| TagKV["Value"] = strings.ReplaceAll(TagKV["Value"], old, newString) | ||
| } |
There was a problem hiding this comment.
Same collision risk persists in runReplaceOnTagsList.
Here, the same nested approach could similarly cause overwritten tag values if multiple transformations merge into the same final key. A single-pass replacement strategy can prevent data loss or collisions:
-for _, TagKV := range values {
- for old, newString := range replacementMap {
- TagKV["Key"] = strings.ReplaceAll(TagKV["Key"], old, newString)
- TagKV["Value"] = strings.ReplaceAll(TagKV["Value"], old, newString)
- }
+for i := range values {
+ newKey := values[i]["Key"]
+ newVal := values[i]["Value"]
+ for old, newString := range replacementMap {
+ newKey = strings.ReplaceAll(newKey, old, newString)
+ newVal = strings.ReplaceAll(newVal, old, newString)
+ }
+ values[i]["Key"] = newKey
+ values[i]["Value"] = newVal
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !config.ReplacementMap.IsNull() && !config.ReplacementMap.IsUnknown() { | |
| replacementMap, _ := framework.FromFrameworkMap[string](ctx, config.ReplacementMap) | |
| for _, TagKV := range values { | |
| for old, newString := range replacementMap { | |
| TagKV["Key"] = strings.ReplaceAll(TagKV["Key"], old, newString) | |
| TagKV["Value"] = strings.ReplaceAll(TagKV["Value"], old, newString) | |
| } | |
| if !config.ReplacementMap.IsNull() && !config.ReplacementMap.IsUnknown() { | |
| replacementMap, _ := framework.FromFrameworkMap[string](ctx, config.ReplacementMap) | |
| for i := range values { | |
| newKey := values[i]["Key"] | |
| newVal := values[i]["Value"] | |
| for old, newString := range replacementMap { | |
| newKey = strings.ReplaceAll(newKey, old, newString) | |
| newVal = strings.ReplaceAll(newVal, old, newString) | |
| } | |
| values[i]["Key"] = newKey | |
| values[i]["Value"] = newVal | |
| } | |
| // ... rest of the code | |
| } |
There was a problem hiding this comment.
This is a good suggestion from @CodeRabbit
There was a problem hiding this comment.
@mcalhoun Thank you for confirming! The single-pass replacement strategy will indeed help prevent potential key collisions and ensure more predictable behavior when multiple replacements are applied. Let me know if you need any further assistance with the implementation.
🧬 Code Definitions Used
Relevant Code Snippets
From internal/provider/constants.go
- Line 19-19
ValidCases = []string{CaseNone, CaseCamel, CaseLower, CaseSnake, CaseTitle, CaseUpper}This snippet defines a constant slice ValidCases that contains valid string representations for tag key and value cases, which are used in the GcpLabelsDataSource schema for validation.
From internal/model/provider_config.go
- Line 19-28
ProviderConfig struct {
delimiter string
enabled bool
properties []Property
propertyOrder []string
replaceCharsRegex string
tagsKeyCase cases.Case
tagsValueCase cases.Case
values map[string]string
}This struct defines the configuration for the provider, including fields for tag key and value cases, which are relevant for the GcpLabelsDataSource operations.
From internal/framework/data_conversion_helpers.go
- Line 11-21
func FromFrameworkMap[T interface{}](ctx context.Context, m types.Map) (map[string]T, diag.Diagnostics) {
localValues := make(map[string]T, len(m.Elements()))
if !m.IsNull() {
diag := m.ElementsAs(ctx, &localValues, false)
if diag.HasError() {
return nil, diag
}
}
return localValues, nil
}This function converts a types.Map to a standard Go map, which is used in the GcpLabelsDataSource to handle the ReplacementMap and other attributes.
From internal/provider/tags_data_source.go
- Line 27-29
TagsDataSource struct {
providerData *model.ProviderData
}This struct serves as a base for GcpLabelsDataSource, holding a reference to provider data, which is essential for tag management.
- Line 32-39
TagsDataSourceModel struct {
Id types.String `tfsdk:"id"`
Values types.Map `tfsdk:"values"`
Tags types.Map `tfsdk:"tags"`
TagsKeyCase types.String `tfsdk:"tags_key_case"`
TagsValueCase types.String `tfsdk:"tags_value_case"`
TagsAsList types.List `tfsdk:"tags_as_list"`
}This model struct defines the data structure for tags, including fields for tag values and cases, which are directly used in the GcpLabelsDataSource.
From pkg/map/hash.go
- Line 10-30
func HashMap(m interface{}) string {
switch v := m.(type) {
case map[string]interface{}:
keys := make([]string, 0, len(v))
for k := range v {
keys = append(keys, k)
}
sort.Strings(keys)
h := sha256.New()
for _, k := range keys {
h.Write([]byte(k))
h.Write([]byte(HashMap(v[k])))
}
return hex.EncodeToString(h.Sum(nil))
default:
// For primitive types, convert to JSON and hash
b, _ := json.Marshal(v)
hashed := sha256.Sum256(b)
return hex.EncodeToString(hashed[:])
}
}This function computes a hash of a map, which is used in the GcpLabelsDataSource to generate a unique identifier for the tags based on their content.
Code Analysis Tools
Tool Analysis Results
CodeRabbit ran static analysis tools on your files and found the following:
🪛 LanguageTool
docs/data-sources/gcp_labels.md
[formatting] ~20-~20: If the ‘applies’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...tring) Map of strings to replace in the tag, applies to both key and value. The key is the s...
(COMMA_AFTER_PREPOSITION_PHRASES)
Files with Issues
docs/data-sources/gcp_labels.md
🪛 LanguageTool
[formatting] ~20-~20: If the ‘applies’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...tring) Map of strings to replace in the tag, applies to both key and value. The key is the s...
(COMMA_AFTER_PREPOSITION_PHRASES)
Next Steps
Based on these results, you can:
- Fix the issues identified by the tools
- Ask me to suggest specific fixes for these issues
- Proceed with planning or implementing changes while accounting for these issues
| lint: | ||
| docker run --rm -v "$(shell pwd):/app" -w /app golangci/golangci-lint:v1.64.8 golangci-lint run --fix |
There was a problem hiding this comment.
IMO, forcing the use of docker here doesn't make sense (it assumes too much). Let's just run golangci-lint directly (it should be a safe assumption that the user has it, as it's a very common tool if you are doing go development. Locally if you personally don't want to install it, you could alias the golangci-lint tool to the docker command above.
There was a problem hiding this comment.
I do not have it, I tried installing it and ran into v2 issues regarding my OS, hence this workaround.
| resp.Diagnostics.Append(resp.State.Set(ctx, &config)...) | ||
| } | ||
|
|
||
| //nolint:revive |
There was a problem hiding this comment.
What is being turned off here? Is there no other way to deal with the lint error?
| config.Id = types.StringValue(tagsAsHash) | ||
| } | ||
|
|
||
| //nolint:revive |
| for tagKey, tagValue := range values { | ||
| for old, newString := range replacementMap { | ||
| newTagKey := strings.ReplaceAll(tagKey, old, newString) | ||
| newTagValue := strings.ReplaceAll(tagValue, old, newString) | ||
| replacedValues[newTagKey] = newTagValue | ||
| } |
There was a problem hiding this comment.
This is a good suggestion from @CodeRabbit
| if !config.ReplacementMap.IsNull() && !config.ReplacementMap.IsUnknown() { | ||
| replacementMap, _ := framework.FromFrameworkMap[string](ctx, config.ReplacementMap) | ||
|
|
||
| for _, TagKV := range values { | ||
| for old, newString := range replacementMap { | ||
| TagKV["Key"] = strings.ReplaceAll(TagKV["Key"], old, newString) | ||
| TagKV["Value"] = strings.ReplaceAll(TagKV["Value"], old, newString) | ||
| } |
There was a problem hiding this comment.
This is a good suggestion from @CodeRabbit
There was a problem hiding this comment.
Consider adding acceptance tests that are using the replacer functionality
There was a problem hiding this comment.
It also looks like you don't have any examples of the new data source for the docs.
This pull request includes significant changes to add a new GCP Labels data source and improve the existing infrastructure. The most important changes include the addition of a new data source, updates to the documentation, and the inclusion of new tests.
New Data Source Addition:
internal/provider/gcp_labels_data_source.go: Added a newGcpLabelsDataSourcewith schema definitions and methods for reading and processing GCP labels.internal/provider/provider.go: Registered the newGcpLabelsDataSourcein the provider's data sources.Documentation Updates:
docs/data-sources/gcp_labels.md: Added documentation for the newcontext_gcp_labelsdata source, including schema details and descriptions of attributes.Testing Enhancements:
internal/provider/gcp_labels_data_source_test.go: Added acceptance tests for theGcpLabelsDataSourceto ensure it functions as expected.Makefile Update:
Makefile: Added a newlocallinttarget to rungolangci-lintfor code linting and fixing issues.