ci: add lint and test workflow for api subpackages#1886
ci: add lint and test workflow for api subpackages#1886
Conversation
- Add api/v1/Makefile with lint, test, build, generate targets - Add api/plugin/Makefile with lint, test, build, proto targets - Add go.mod files for independent module builds - Add api/plugin infrastructure (proto, grpc, types)
WalkthroughAdds a gRPC-based scraper plugin system (proto, host services, client/server), bidirectional protobuf↔domain conversion with tests, new api/v1 and api/plugin modules and Makefiles, widespread import-path and JSON Schema ID updates, dependency updates, and a CI workflow to lint/test the api modules. Changes
Sequence Diagram(s)sequenceDiagram
participant Host as HostProcess
participant Broker as GRPCBroker
participant Client as grpcClient
participant Server as grpcServer
participant Impl as ScraperPluginImpl
participant HostSvc as HostServicesServer
Host->>Broker: request plugin connection
Broker->>Client: provide pb.ScraperPluginClient + HostServicesClient
Host->>Client: Scrape(spec_json, scraper_id, namespace)
Client->>Server: pb.Scrape RPC
Server->>Impl: forward Scrape call (domain types)
Impl->>HostSvc: HydrateConnection / GetEnvValue / FindConfig
HostSvc-->>Impl: return connection/env/config
Impl-->>Server: return domain ScrapeResult(s)
Server->>Client: pb.ScrapeResponse
Client-->>Host: convert proto -> domain and return results
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 9
🤖 Fix all issues with AI agents
In `@api/plugin/convert.go`:
- Around line 287-290: The json.Marshal call for c.Details in convert.go
currently ignores errors; update the block that marshals c.Details (the
json.Marshal(c.Details) near where result.Details is assigned) to capture the
returned error, handle it appropriately (return the error from the surrounding
function or log and propagate it), and only assign result.Details when err ==
nil; ensure you use the existing surrounding function's error-handling pattern
so callers can observe/fail on marshal failures.
- Around line 208-211: The json.Marshal call for a.Analysis in convert.go
silently ignores errors; update the block around json.Marshal(a.Analysis) so you
capture the returned error, handle it (return the error up the call chain or log
it and wrap it) instead of discarding it, and only assign to result.Analysis
when marshal succeeds; specifically change the logic where a.Analysis is checked
and the marshal is performed so the error is checked and propagated (or logged)
using the surrounding function’s error handling flow.
- Around line 396-410: Several protoTo* converters (protoToExternalRole,
protoToExternalUser, protoToExternalGroup, protoToExternalUserGroup,
protoToExternalConfigAccess, protoToExternalConfigAccessLog) call uuid.MustParse
which can panic on malformed input; replace those MustParse calls with
uuid.Parse and handle the error by only assigning the parsed UUID when err ==
nil (for pointer fields set to &id) and otherwise leave the field zero-valued or
nil and optionally log/return an error at the proto boundary if you prefer
stricter validation. Locate each function (protoToExternalRole,
protoToExternalUser, protoToExternalGroup, protoToExternalUserGroup,
protoToExternalConfigAccess, protoToExternalConfigAccessLog) and change
uuid.MustParse(...) to id, err := uuid.Parse(...); if err == nil { ...assign...
} to avoid panics.
- Around line 13-14: The api/v1 module is missing the
github.com/flanksource/config-db/utils dependency required by types.go and
interface.go; update the api/v1 module definition (api/v1/go.mod) to include
that dependency by either adding a require for
github.com/flanksource/config-db/utils or, for local development, adding a
replace directive pointing to the local path (e.g., replace
github.com/flanksource/config-db/utils => ../../utils) so that builds
referencing types.go and interface.go succeed.
In `@api/plugin/proto/scraper.proto`:
- Around line 3-5: The proto package declaration "package configdb.plugin;" and
its accompanying "option go_package" are inconsistent with the file's directory,
triggering Buf's package-directory rule; fix by either updating the package to
reflect the directory (e.g., change the 'package' to a matching name such as
"api.plugin" or "api.plugin.proto" and update the 'option go_package' value to
the corresponding Go import path), or move this .proto into a directory matching
"configdb/plugin" so the existing "package configdb.plugin" is correct; choose
one approach and make the package and option go_package consistent with the file
location (or alternatively add a Buf rule to ignore this file if the layout is
intentional).
- Around line 192-200: The proto message ExternalConfigAccessLogProto declares
fields id, operation, and ip_address that are never mapped in the conversion
helpers; either remove these fields from the proto or add mapping in
externalConfigAccessLogToProto and protoToExternalConfigAccessLog so the data
flows correctly. If you choose mapping, set ExternalConfigAccessLogProto.id,
.operation and .ip_address from the domain object fields (e.g., source.ID /
source.Operation / source.IPAddress or their equivalents) in
externalConfigAccessLogToProto, and populate the domain object from proto.id,
proto.operation and proto.ip_address in protoToExternalConfigAccessLog; keep
existing mappings for config_id, external_user_id, created_at and
config_external_id intact. Ensure null/empty handling and types (string/int64)
match between the proto and domain model.
In `@api/plugin/types.go`:
- Around line 50-70: The EnvVar.ToDuty method fails to copy the env var name,
causing the returned types.EnvVar to lose its Name; update the conversion in
EnvVar.ToDuty (function ToDuty) to set ev.Name = e.Name (for example initialize
types.EnvVar with Name: e.Name or assign ev.Name before return) so the name is
preserved during the round-trip conversion.
- Around line 35-48: The conversion in EnvVarFromDuty omits copying the EnvVar
name, so update the EnvVar construction in EnvVarFromDuty to set the Name field
from the source (e.Name) when building the ev variable; ensure EnvVarFromDuty
assigns ev.Name = e.Name (in addition to ValueStatic and the existing
ValueFromConfigMapKeyRef/SecretKeyRef logic) so the EnvVar struct's Name is
preserved.
In `@api/v1/go.mod`:
- Around line 1-3: Update the module declaration in the api/v1 module from
"github.com/flanksource/config-db/api" to
"github.com/flanksource/config-db/api/v1" and then update the dependent module
reference in api/plugin's go.mod from "github.com/flanksource/config-db/api
v0.0.0" to "github.com/flanksource/config-db/api/v1 v0.0.0" (or remove/version
as appropriate); after changing the module path strings, run "go mod tidy" in
both the api/v1 and api/plugin directories and fix any import paths in code that
refer to "github.com/flanksource/config-db/api" so they use
"github.com/flanksource/config-db/api/v1" consistently.
🧹 Nitpick comments (4)
api/v1/Makefile (1)
11-48: Optional: addall/cleanphony targets to satisfy checkmake.This silences the reported warnings and keeps the Makefile conventional.
♻️ Suggested addition
+.PHONY: all +all: lint test + +.PHONY: clean +clean: + rm -rf $(LOCALBIN)api/plugin/Makefile (1)
9-45: Optional: addall/cleanphony targets to satisfy checkmake.Keeps lint quiet and matches common Makefile conventions.
♻️ Suggested addition
+.PHONY: all +all: lint test + +.PHONY: clean +clean: + rm -rf $(LOCALBIN)api/plugin/grpc_plugin.go (2)
18-22:HostServicesfield is declared but unused.The
HostServicesfield inGRPCPluginis never referenced inGRPCServerorGRPCClientmethods. If this is intended for future use, consider adding a TODO comment; otherwise, remove it to avoid confusion.
29-38: Hardcoded broker ID and unusedhostClient.Two observations:
broker.Dial(1)uses a hardcoded broker ID. Ensure the host side callsbroker.AcceptAndServe(1, ...)with the same ID. Consider extracting this as a constant for clarity.The
hostClientis stored ingrpcClientbut there's no method to access it. If plugins need to call host services, expose this via a getter or interface method.♻️ Suggested improvement
+const hostServiceBrokerID = 1 + func (p *GRPCPlugin) GRPCClient(ctx context.Context, broker *plugin.GRPCBroker, c *grpc.ClientConn) (interface{}, error) { - hostConn, err := broker.Dial(1) + hostConn, err := broker.Dial(hostServiceBrokerID) if err != nil { return nil, err }If
hostClientis needed by plugins, add a method:func (c *grpcClient) HostServices() pb.HostServicesClient { return c.hostClient }
| v1 "github.com/flanksource/config-db/api" | ||
| pb "github.com/flanksource/config-db/api/plugin/proto" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for utils imports in the api package
rg -n "config-db/utils" api/ --type goRepository: flanksource/config-db
Length of output: 189
🏁 Script executed:
#!/bin/bash
# Check the go.mod files to understand module structure
echo "=== api/go.mod ==="
if [ -f api/go.mod ]; then cat -n api/go.mod; fi
echo -e "\n=== api/plugin/go.mod ==="
if [ -f api/plugin/go.mod ]; then cat -n api/plugin/go.mod; fi
echo -e "\n=== Root go.mod ==="
if [ -f go.mod ]; then cat -n go.mod | head -100; fiRepository: flanksource/config-db
Length of output: 29946
🏁 Script executed:
#!/bin/bash
# List the directory structure to understand module boundaries
ls -la api/
ls -la api/plugin/Repository: flanksource/config-db
Length of output: 1210
🏁 Script executed:
#!/bin/bash
# Check if api/v1 has its own go.mod
ls -la api/v1/ | grep go.modRepository: flanksource/config-db
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Search for utils in root go.mod and check if it's a direct dependency
rg -n "utils" go.modRepository: flanksource/config-db
Length of output: 663
🏁 Script executed:
#!/bin/bash
# Get the first part of api/v1/types.go and interface.go to see their imports
echo "=== api/v1/types.go (first 15 lines) ==="
head -n 15 api/v1/types.go
echo -e "\n=== api/v1/interface.go (first 30 lines) ==="
head -n 30 api/v1/interface.goRepository: flanksource/config-db
Length of output: 1059
🏁 Script executed:
#!/bin/bash
# Check api/v1/go.mod for utils dependency
echo "=== api/v1/go.mod (checking for utils) ==="
grep -n "utils" api/v1/go.mod
echo -e "\n=== api/v1/go.mod (first 30 lines) ==="
head -n 30 api/v1/go.modRepository: flanksource/config-db
Length of output: 1678
🏁 Script executed:
#!/bin/bash
# Check if utils package/module exists in the repo
ls -la | grep -E "^d.*utils"
fd -t d "^utils$" .Repository: flanksource/config-db
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Show the full import section of convert.go
echo "=== api/plugin/convert.go (first 20 lines) ==="
head -n 20 api/plugin/convert.goRepository: flanksource/config-db
Length of output: 588
🏁 Script executed:
#!/bin/bash
# Check if api/v1/go.mod has a replace directive for utils
grep -n "^replace" api/v1/go.modRepository: flanksource/config-db
Length of output: 47
Resolve missing dependency for github.com/flanksource/config-db/utils in the api/v1 module.
The api/v1 module imports github.com/flanksource/config-db/utils in types.go and interface.go, but api/v1/go.mod does not declare this as a requirement or provide a replace directive. This causes the build to fail when api/plugin depends on api/v1.
Add one of the following to api/v1/go.mod:
require github.com/flanksource/config-db/utils(direct dependency)- A replace directive:
replace github.com/flanksource/config-db/utils => ../../utils
🧰 Tools
🪛 GitHub Actions: API Lint & Test
[error] 13-13: could not import github.com/flanksource/config-db/api (import in convert.go) due to missing module dependencies (no required module provides package github.com/flanksource/config-db/utils)
🤖 Prompt for AI Agents
In `@api/plugin/convert.go` around lines 13 - 14, The api/v1 module is missing the
github.com/flanksource/config-db/utils dependency required by types.go and
interface.go; update the api/v1 module definition (api/v1/go.mod) to include
that dependency by either adding a require for
github.com/flanksource/config-db/utils or, for local development, adding a
replace directive pointing to the local path (e.g., replace
github.com/flanksource/config-db/utils => ../../utils) so that builds
referencing types.go and interface.go succeed.
| if a.Analysis != nil { | ||
| analysisJSON, _ := json.Marshal(a.Analysis) | ||
| result.Analysis = analysisJSON | ||
| } |
There was a problem hiding this comment.
Silently ignored JSON marshal error.
The error from json.Marshal(a.Analysis) is discarded. If marshaling fails, result.Analysis will be nil, potentially losing data without any indication.
🛠️ Proposed fix
if a.Analysis != nil {
- analysisJSON, _ := json.Marshal(a.Analysis)
- result.Analysis = analysisJSON
+ analysisJSON, err := json.Marshal(a.Analysis)
+ if err != nil {
+ logger.Warnf("failed to marshal analysis: %v", err)
+ } else {
+ result.Analysis = analysisJSON
+ }
}📝 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 a.Analysis != nil { | |
| analysisJSON, _ := json.Marshal(a.Analysis) | |
| result.Analysis = analysisJSON | |
| } | |
| if a.Analysis != nil { | |
| analysisJSON, err := json.Marshal(a.Analysis) | |
| if err != nil { | |
| logger.Warnf("failed to marshal analysis: %v", err) | |
| } else { | |
| result.Analysis = analysisJSON | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@api/plugin/convert.go` around lines 208 - 211, The json.Marshal call for
a.Analysis in convert.go silently ignores errors; update the block around
json.Marshal(a.Analysis) so you capture the returned error, handle it (return
the error up the call chain or log it and wrap it) instead of discarding it, and
only assign to result.Analysis when marshal succeeds; specifically change the
logic where a.Analysis is checked and the marshal is performed so the error is
checked and propagated (or logged) using the surrounding function’s error
handling flow.
| if c.Details != nil { | ||
| detailsJSON, _ := json.Marshal(c.Details) | ||
| result.Details = detailsJSON | ||
| } |
There was a problem hiding this comment.
Silently ignored JSON marshal error.
Same issue as above—the error from json.Marshal(c.Details) is discarded.
🛠️ Proposed fix
if c.Details != nil {
- detailsJSON, _ := json.Marshal(c.Details)
- result.Details = detailsJSON
+ detailsJSON, err := json.Marshal(c.Details)
+ if err != nil {
+ logger.Warnf("failed to marshal details: %v", err)
+ } else {
+ result.Details = detailsJSON
+ }
}📝 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 c.Details != nil { | |
| detailsJSON, _ := json.Marshal(c.Details) | |
| result.Details = detailsJSON | |
| } | |
| if c.Details != nil { | |
| detailsJSON, err := json.Marshal(c.Details) | |
| if err != nil { | |
| logger.Warnf("failed to marshal details: %v", err) | |
| } else { | |
| result.Details = detailsJSON | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@api/plugin/convert.go` around lines 287 - 290, The json.Marshal call for
c.Details in convert.go currently ignores errors; update the block that marshals
c.Details (the json.Marshal(c.Details) near where result.Details is assigned) to
capture the returned error, handle it appropriately (return the error from the
surrounding function or log and propagate it), and only assign result.Details
when err == nil; ensure you use the existing surrounding function's
error-handling pattern so callers can observe/fail on marshal failures.
| func protoToExternalRole(p *pb.ExternalRoleProto) models.ExternalRole { | ||
| role := models.ExternalRole{ | ||
| Name: p.Name, | ||
| AccountID: p.AccountId, | ||
| Description: p.Description, | ||
| } | ||
| if p.Id != "" { | ||
| role.ID = uuid.MustParse(p.Id) | ||
| } | ||
| if p.ScraperId != "" { | ||
| id := uuid.MustParse(p.ScraperId) | ||
| role.ScraperID = &id | ||
| } | ||
| return role | ||
| } |
There was a problem hiding this comment.
uuid.MustParse can panic on invalid input.
Multiple protoTo* functions use uuid.MustParse which will panic if the string is not a valid UUID. If the proto message contains malformed data from an external plugin, this will crash the host.
Consider using uuid.Parse with error handling, or at minimum validate at the proto boundary.
🛠️ Proposed fix pattern
- if p.Id != "" {
- role.ID = uuid.MustParse(p.Id)
- }
+ if p.Id != "" {
+ id, err := uuid.Parse(p.Id)
+ if err != nil {
+ logger.Warnf("invalid UUID for role ID: %v", err)
+ } else {
+ role.ID = id
+ }
+ }This pattern should be applied to all uuid.MustParse calls in:
protoToExternalRole(lines 403, 406)protoToExternalUser(lines 437, 440)protoToExternalGroup(lines 477, 480)protoToExternalUserGroup(lines 501, 502)protoToExternalConfigAccess(lines 544, 547, 551, 555, 559)protoToExternalConfigAccessLog(lines 596, 599)
📝 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.
| func protoToExternalRole(p *pb.ExternalRoleProto) models.ExternalRole { | |
| role := models.ExternalRole{ | |
| Name: p.Name, | |
| AccountID: p.AccountId, | |
| Description: p.Description, | |
| } | |
| if p.Id != "" { | |
| role.ID = uuid.MustParse(p.Id) | |
| } | |
| if p.ScraperId != "" { | |
| id := uuid.MustParse(p.ScraperId) | |
| role.ScraperID = &id | |
| } | |
| return role | |
| } | |
| func protoToExternalRole(p *pb.ExternalRoleProto) models.ExternalRole { | |
| role := models.ExternalRole{ | |
| Name: p.Name, | |
| AccountID: p.AccountId, | |
| Description: p.Description, | |
| } | |
| if p.Id != "" { | |
| id, err := uuid.Parse(p.Id) | |
| if err != nil { | |
| logger.Warnf("invalid UUID for role ID: %v", err) | |
| } else { | |
| role.ID = id | |
| } | |
| } | |
| if p.ScraperId != "" { | |
| id := uuid.MustParse(p.ScraperId) | |
| role.ScraperID = &id | |
| } | |
| return role | |
| } |
🤖 Prompt for AI Agents
In `@api/plugin/convert.go` around lines 396 - 410, Several protoTo* converters
(protoToExternalRole, protoToExternalUser, protoToExternalGroup,
protoToExternalUserGroup, protoToExternalConfigAccess,
protoToExternalConfigAccessLog) call uuid.MustParse which can panic on malformed
input; replace those MustParse calls with uuid.Parse and handle the error by
only assigning the parsed UUID when err == nil (for pointer fields set to &id)
and otherwise leave the field zero-valued or nil and optionally log/return an
error at the proto boundary if you prefer stricter validation. Locate each
function (protoToExternalRole, protoToExternalUser, protoToExternalGroup,
protoToExternalUserGroup, protoToExternalConfigAccess,
protoToExternalConfigAccessLog) and change uuid.MustParse(...) to id, err :=
uuid.Parse(...); if err == nil { ...assign... } to avoid panics.
| package configdb.plugin; | ||
|
|
||
| option go_package = "github.com/flanksource/config-db/api/plugin/proto"; |
There was a problem hiding this comment.
Proto package doesn't match directory structure (Buf lint error).
Buf reports that files with package configdb.plugin should be in a configdb/plugin directory, but this file is in api/plugin/proto/.
Options to resolve:
- Change package to
api.pluginorapi.plugin.prototo match the directory - Move the file to
configdb/plugin/scraper.proto - Configure Buf to ignore this rule if the current structure is intentional
♻️ Option 1: Update package name
-package configdb.plugin;
+package api.plugin;📝 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.
| package configdb.plugin; | |
| option go_package = "github.com/flanksource/config-db/api/plugin/proto"; | |
| package api.plugin; | |
| option go_package = "github.com/flanksource/config-db/api/plugin/proto"; |
🧰 Tools
🪛 Buf (1.64.0)
[error] 3-3: Files with package "configdb.plugin" must be within a directory "configdb/plugin" relative to root but were in directory "api/plugin/proto".
(PACKAGE_DIRECTORY_MATCH)
🤖 Prompt for AI Agents
In `@api/plugin/proto/scraper.proto` around lines 3 - 5, The proto package
declaration "package configdb.plugin;" and its accompanying "option go_package"
are inconsistent with the file's directory, triggering Buf's package-directory
rule; fix by either updating the package to reflect the directory (e.g., change
the 'package' to a matching name such as "api.plugin" or "api.plugin.proto" and
update the 'option go_package' value to the corresponding Go import path), or
move this .proto into a directory matching "configdb/plugin" so the existing
"package configdb.plugin" is correct; choose one approach and make the package
and option go_package consistent with the file location (or alternatively add a
Buf rule to ignore this file if the layout is intentional).
| message ExternalConfigAccessLogProto { | ||
| string id = 1; | ||
| string config_id = 2; | ||
| string external_user_id = 3; | ||
| string operation = 4; | ||
| string ip_address = 5; | ||
| int64 created_at = 6; | ||
| ExternalIDProto config_external_id = 7; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the convert.go file related to ExternalConfigAccessLog
find . -name "convert.go" -type f | head -20Repository: flanksource/config-db
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check the ExternalConfigAccessLog struct definition in the domain model
find . -name "*.go" -type f | xargs grep -l "type ExternalConfigAccessLog struct" | head -5Repository: flanksource/config-db
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Search for conversion functions
find . -name "*.go" -type f | xargs grep -l "externalConfigAccessLogToProto\|protoToExternalConfigAccessLog" | head -10Repository: flanksource/config-db
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check the ExternalConfigAccessLog struct definition
grep -A 30 "type ExternalConfigAccessLog struct" ./api/v1/interface.goRepository: flanksource/config-db
Length of output: 849
🏁 Script executed:
#!/bin/bash
# Find and read the conversion functions for ExternalConfigAccessLog
grep -n "externalConfigAccessLogToProto\|protoToExternalConfigAccessLog" ./api/plugin/convert.goRepository: flanksource/config-db
Length of output: 477
🏁 Script executed:
#!/bin/bash
# Read the conversion functions
sed -n '579,610p' ./api/plugin/convert.goRepository: flanksource/config-db
Length of output: 1138
🏁 Script executed:
#!/bin/bash
# Read the reverse conversion function
sed -n '593,620p' ./api/plugin/convert.goRepository: flanksource/config-db
Length of output: 661
🏁 Script executed:
#!/bin/bash
# Find ConfigAccessLog model definition
find . -name "*.go" -type f | xargs grep -l "type ConfigAccessLog struct" | head -5Repository: flanksource/config-db
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Broader search for ConfigAccessLog definition
rg "type ConfigAccessLog struct" -A 15Repository: flanksource/config-db
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check if id, operation, or ip_address from ExternalConfigAccessLogProto are referenced anywhere
rg "\.Id\b|\.Operation\b|\.IpAddress\b" ./api/plugin/convert.go | head -20Repository: flanksource/config-db
Length of output: 250
🏁 Script executed:
#!/bin/bash
# Check proto file to understand field definitions better
sed -n '192,200p' ./api/plugin/proto/scraper.protoRepository: flanksource/config-db
Length of output: 292
🏁 Script executed:
#!/bin/bash
# Get complete externalConfigAccessLogToProto function
sed -n '579,591p' ./api/plugin/convert.goRepository: flanksource/config-db
Length of output: 554
🏁 Script executed:
#!/bin/bash
# Get complete protoToExternalConfigAccessLog function
sed -n '593,610p' ./api/plugin/convert.goRepository: flanksource/config-db
Length of output: 647
🏁 Script executed:
#!/bin/bash
# Search for any reference to id, operation, or ip_address in context of ExternalConfigAccessLog
rg "ExternalConfigAccessLog|\.Operation\b|\.IpAddress\b" ./api/plugin/convert.go -B 2 -A 2Repository: flanksource/config-db
Length of output: 691
Remove or map unused fields in ExternalConfigAccessLogProto.
Fields id, operation, and ip_address are defined in the proto message but never mapped in the conversion functions externalConfigAccessLogToProto or protoToExternalConfigAccessLog. Either remove these fields from the proto if they are not needed, or add the necessary mapping logic in the conversion functions.
🤖 Prompt for AI Agents
In `@api/plugin/proto/scraper.proto` around lines 192 - 200, The proto message
ExternalConfigAccessLogProto declares fields id, operation, and ip_address that
are never mapped in the conversion helpers; either remove these fields from the
proto or add mapping in externalConfigAccessLogToProto and
protoToExternalConfigAccessLog so the data flows correctly. If you choose
mapping, set ExternalConfigAccessLogProto.id, .operation and .ip_address from
the domain object fields (e.g., source.ID / source.Operation / source.IPAddress
or their equivalents) in externalConfigAccessLogToProto, and populate the domain
object from proto.id, proto.operation and proto.ip_address in
protoToExternalConfigAccessLog; keep existing mappings for config_id,
external_user_id, created_at and config_external_id intact. Ensure null/empty
handling and types (string/int64) match between the proto and domain model.
| func EnvVarFromDuty(e types.EnvVar) EnvVar { | ||
| ev := EnvVar{ | ||
| ValueStatic: e.ValueStatic, | ||
| } | ||
| if e.ValueFrom != nil && e.ValueFrom.ConfigMapKeyRef != nil { | ||
| ev.ValueFromConfigMapKeyRefName = e.ValueFrom.ConfigMapKeyRef.Name | ||
| ev.ValueFromConfigMapKeyRefKey = e.ValueFrom.ConfigMapKeyRef.Key | ||
| } | ||
| if e.ValueFrom != nil && e.ValueFrom.SecretKeyRef != nil { | ||
| ev.ValueFromSecretKeyRefName = e.ValueFrom.SecretKeyRef.Name | ||
| ev.ValueFromSecretKeyRefKey = e.ValueFrom.SecretKeyRef.Key | ||
| } | ||
| return ev | ||
| } |
There was a problem hiding this comment.
Missing Name field in EnvVarFromDuty conversion.
The EnvVar struct has a Name field (line 27), but EnvVarFromDuty does not copy e.Name from the input. This will cause the name to be lost during conversion.
🐛 Proposed fix
func EnvVarFromDuty(e types.EnvVar) EnvVar {
ev := EnvVar{
+ Name: e.Name,
ValueStatic: e.ValueStatic,
}🤖 Prompt for AI Agents
In `@api/plugin/types.go` around lines 35 - 48, The conversion in EnvVarFromDuty
omits copying the EnvVar name, so update the EnvVar construction in
EnvVarFromDuty to set the Name field from the source (e.Name) when building the
ev variable; ensure EnvVarFromDuty assigns ev.Name = e.Name (in addition to
ValueStatic and the existing ValueFromConfigMapKeyRef/SecretKeyRef logic) so the
EnvVar struct's Name is preserved.
| func (e EnvVar) ToDuty() types.EnvVar { | ||
| ev := types.EnvVar{ | ||
| ValueStatic: e.ValueStatic, | ||
| } | ||
| if e.ValueFromConfigMapKeyRefName != "" || e.ValueFromSecretKeyRefName != "" { | ||
| ev.ValueFrom = &types.EnvVarSource{} | ||
| } | ||
| if e.ValueFromConfigMapKeyRefName != "" { | ||
| ev.ValueFrom.ConfigMapKeyRef = &types.ConfigMapKeySelector{ | ||
| LocalObjectReference: types.LocalObjectReference{Name: e.ValueFromConfigMapKeyRefName}, | ||
| Key: e.ValueFromConfigMapKeyRefKey, | ||
| } | ||
| } | ||
| if e.ValueFromSecretKeyRefName != "" { | ||
| ev.ValueFrom.SecretKeyRef = &types.SecretKeySelector{ | ||
| LocalObjectReference: types.LocalObjectReference{Name: e.ValueFromSecretKeyRefName}, | ||
| Key: e.ValueFromSecretKeyRefKey, | ||
| } | ||
| } | ||
| return ev | ||
| } |
There was a problem hiding this comment.
Missing Name field in ToDuty conversion.
Similarly, the ToDuty method does not copy e.Name back to the returned types.EnvVar, which will lose the name during round-trip conversion.
🐛 Proposed fix
func (e EnvVar) ToDuty() types.EnvVar {
ev := types.EnvVar{
+ Name: e.Name,
ValueStatic: e.ValueStatic,
}📝 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.
| func (e EnvVar) ToDuty() types.EnvVar { | |
| ev := types.EnvVar{ | |
| ValueStatic: e.ValueStatic, | |
| } | |
| if e.ValueFromConfigMapKeyRefName != "" || e.ValueFromSecretKeyRefName != "" { | |
| ev.ValueFrom = &types.EnvVarSource{} | |
| } | |
| if e.ValueFromConfigMapKeyRefName != "" { | |
| ev.ValueFrom.ConfigMapKeyRef = &types.ConfigMapKeySelector{ | |
| LocalObjectReference: types.LocalObjectReference{Name: e.ValueFromConfigMapKeyRefName}, | |
| Key: e.ValueFromConfigMapKeyRefKey, | |
| } | |
| } | |
| if e.ValueFromSecretKeyRefName != "" { | |
| ev.ValueFrom.SecretKeyRef = &types.SecretKeySelector{ | |
| LocalObjectReference: types.LocalObjectReference{Name: e.ValueFromSecretKeyRefName}, | |
| Key: e.ValueFromSecretKeyRefKey, | |
| } | |
| } | |
| return ev | |
| } | |
| func (e EnvVar) ToDuty() types.EnvVar { | |
| ev := types.EnvVar{ | |
| Name: e.Name, | |
| ValueStatic: e.ValueStatic, | |
| } | |
| if e.ValueFromConfigMapKeyRefName != "" || e.ValueFromSecretKeyRefName != "" { | |
| ev.ValueFrom = &types.EnvVarSource{} | |
| } | |
| if e.ValueFromConfigMapKeyRefName != "" { | |
| ev.ValueFrom.ConfigMapKeyRef = &types.ConfigMapKeySelector{ | |
| LocalObjectReference: types.LocalObjectReference{Name: e.ValueFromConfigMapKeyRefName}, | |
| Key: e.ValueFromConfigMapKeyRefKey, | |
| } | |
| } | |
| if e.ValueFromSecretKeyRefName != "" { | |
| ev.ValueFrom.SecretKeyRef = &types.SecretKeySelector{ | |
| LocalObjectReference: types.LocalObjectReference{Name: e.ValueFromSecretKeyRefName}, | |
| Key: e.ValueFromSecretKeyRefKey, | |
| } | |
| } | |
| return ev | |
| } |
🤖 Prompt for AI Agents
In `@api/plugin/types.go` around lines 50 - 70, The EnvVar.ToDuty method fails to
copy the env var name, causing the returned types.EnvVar to lose its Name;
update the conversion in EnvVar.ToDuty (function ToDuty) to set ev.Name = e.Name
(for example initialize types.EnvVar with Name: e.Name or assign ev.Name before
return) so the name is preserved during the round-trip conversion.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@api/plugin/convert_test.go`:
- Line 8: The import line in convert_test.go uses a duplicate alias ("v1 v1
\"github.com/flanksource/config-db/api\""), causing a syntax error; fix it by
removing the extra alias so the import reads with a single alias (e.g., change
to v1 "github.com/flanksource/config-db/api" or use the bare path) — locate the
import block in convert_test.go and correct the import alias for the package.
In `@api/plugin/convert.go`:
- Around line 13-14: The import line in convert.go uses an invalid duplicate
alias `v1 v1 "github.com/flanksource/config-db/api"`; fix the import statement
by removing the duplicate alias so it reads with a single alias (e.g., `v1
"github.com/flanksource/config-db/api"`) or by importing the package without an
explicit alias, ensuring the rest of the file that references the package
(symbols using v1) still compile; update the import block near the existing `pb
"github.com/flanksource/config-db/api/plugin/proto"` entry accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@api/plugin/convert_test.go`:
- Around line 164-179: Rename or split the test to reflect what it's actually
verifying: update the test function name TestProtoToChangeResultWithEmptyDetails
to TestProtoToChangeResultWithJSONDetails (or add a new test named
TestProtoToChangeResultWithEmptyDetails that covers nil/empty Details) and
ensure you're exercising protoToChangeResult accordingly; keep the existing
assertions for the JSON case under the new JSON-specific test and add a separate
case that passes nil/empty Details to verify the function handles empty detail
bytes.
🧹 Nitpick comments (1)
api/plugin/convert_test.go (1)
42-56: Consider verifying all populated fields after round-trip.The round-trip verification only checks
ID,Name, andType, but the original also setsConfigClass,Status,Health,Tags, andLabels. Adding assertions for these fields would improve confidence in the conversion logic.💡 Suggested additional assertions
if roundTrip.Type != original.Type { t.Errorf("RoundTrip Type mismatch: got %s, want %s", roundTrip.Type, original.Type) } + if roundTrip.ConfigClass != original.ConfigClass { + t.Errorf("RoundTrip ConfigClass mismatch: got %s, want %s", roundTrip.ConfigClass, original.ConfigClass) + } + if roundTrip.Status != original.Status { + t.Errorf("RoundTrip Status mismatch: got %s, want %s", roundTrip.Status, original.Status) + } + if roundTrip.Health != original.Health { + t.Errorf("RoundTrip Health mismatch: got %s, want %s", roundTrip.Health, original.Health) + } }
| func TestProtoToChangeResultWithEmptyDetails(t *testing.T) { | ||
| proto := &pb.ChangeResultProto{ | ||
| ExternalId: "test", | ||
| ChangeType: "UPDATE", | ||
| Details: []byte(`{"key": "value"}`), | ||
| } | ||
|
|
||
| result := protoToChangeResult(proto) | ||
|
|
||
| if result.Details == nil { | ||
| t.Error("Details should be unmarshaled") | ||
| } | ||
| if result.Details["key"] != "value" { | ||
| t.Errorf("Details[key] mismatch: got %v, want 'value'", result.Details["key"]) | ||
| } | ||
| } |
There was a problem hiding this comment.
Misleading test function name.
The test is named TestProtoToChangeResultWithEmptyDetails but actually tests with valid JSON details {"key": "value"}. Consider renaming to TestProtoToChangeResultWithJSONDetails or adding a separate test case for truly empty details.
💡 Suggested rename
-func TestProtoToChangeResultWithEmptyDetails(t *testing.T) {
+func TestProtoToChangeResultWithJSONDetails(t *testing.T) {🤖 Prompt for AI Agents
In `@api/plugin/convert_test.go` around lines 164 - 179, Rename or split the test
to reflect what it's actually verifying: update the test function name
TestProtoToChangeResultWithEmptyDetails to
TestProtoToChangeResultWithJSONDetails (or add a new test named
TestProtoToChangeResultWithEmptyDetails that covers nil/empty Details) and
ensure you're exercising protoToChangeResult accordingly; keep the existing
assertions for the JSON case under the new JSON-specific test and add a separate
case that passes nil/empty Details to verify the function handles empty detail
bytes.
1 participant
Summary by CodeRabbit
New Features
Tests
Chores