feat(CC-0004): add common types and pure function packages#7
Merged
Conversation
berendt
added a commit
that referenced
this pull request
Mar 1, 2026
Reviewer's GuideIntroduces a new internal/common module with shared CRD-embeddable types and pure-function helper packages for conditions, INI config generation, PasteDeploy plugin/pipeline rendering, and policy rule handling, all covered by table-driven tests and referenced in new docs. Class diagram for shared internal common typesclassDiagram
class ImageSpec {
string Repository
string Tag
}
class SecretRefSpec {
string Name
string Key
}
class corev1_LocalObjectReference {
string Name
}
class DatabaseSpec {
corev1_LocalObjectReference ClusterRef
string Host
int32 Port
string Database
SecretRefSpec SecretRef
}
class MessagingSpec {
string Host
int32 Port
SecretRefSpec SecretRef
string Vhost
}
class CacheSpec {
corev1_LocalObjectReference ClusterRef
string[] Servers
}
class PolicySpec {
corev1_LocalObjectReference ConfigMapRef
map~string,string~ Inline
}
class PluginSpec {
string Name
string Section
map~string,string~ Config
}
class PipelinePosition {
string Before
string After
}
class MiddlewareSpec {
string Name
string FilterFactory
map~string,string~ Config
PipelinePosition Position
}
class FilterSpec {
string Name
string Factory
map~string,string~ Config
}
class PipelineSpec {
string Name
string BasePipeline
FilterSpec[] Filters
MiddlewareSpec[] Middleware
}
DatabaseSpec --> SecretRefSpec : uses
DatabaseSpec --> corev1_LocalObjectReference : ClusterRef
MessagingSpec --> SecretRefSpec : uses
CacheSpec --> corev1_LocalObjectReference : ClusterRef
PolicySpec --> corev1_LocalObjectReference : ConfigMapRef
MiddlewareSpec --> PipelinePosition : embeds
PipelineSpec --> FilterSpec : contains
PipelineSpec --> MiddlewareSpec : contains
Class diagram for internal common utility packages and functionsclassDiagram
class conditions_Package {
<<utility>>
+SetCondition(conditions, conditionType, status, reason, message) void
+GetCondition(conditions, conditionType) metav1_Condition_ptr
+IsReady(conditions) bool
+AllTrue(conditions, types) bool
}
class config_Package {
<<utility>>
+RenderINI(config) string
+MergeDefaults(userConfig, defaults) map_string_map_string_string
+InjectSecrets(config, secrets) map_string_map_string_string
+InjectOsloPolicyConfig(config, policyFilePath) map_string_map_string_string
}
class plugins_Package {
<<utility>>
+RenderPastePipeline(spec) string
+RenderPluginConfig(plugins) map_string_map_string_string
}
class policy_Package {
<<utility>>
+MergePolicies(external, inline) map_string_string
+ValidatePolicyRules(rules, fldPath) field_ErrorList
+RenderPolicyYAML(rules) string_or_error
}
class metav1_Condition {
string Type
string Status
string Reason
string Message
time_Time LastTransitionTime
}
class types_PipelineSpec {
}
class types_PluginSpec {
}
class field_Path {
}
class field_ErrorList {
}
conditions_Package --> metav1_Condition : operates_on
plugins_Package --> types_PipelineSpec : parameter
plugins_Package --> types_PluginSpec : parameter
policy_Package --> field_Path : uses
policy_Package --> field_ErrorList : returns
Flow diagram for reconciler config and policy pipelineflowchart TD
A[CRD_Spec_Fields] --> B[MergeDefaults]
D[Operator_Defaults] --> B
B --> C[InjectSecrets]
E[Resolved_Secrets] --> C
C --> F[InjectOsloPolicyConfig]
G[Policy_File_Path] --> F
F --> H[RenderINI]
H --> I[Main_INI_ConfigMap]
A --> J[RenderPluginConfig]
J --> K[Plugin_Config_Map]
K --> L[MergeDefaults_Combined]
H --> L
L --> M[Final_INI_ConfigMap]
A --> N[RenderPastePipeline]
N --> O[api_paste_ini_ConfigMap]
A --> P[MergePolicies]
Q[External_Policy_ConfigMap_Rules] --> P
P --> R[ValidatePolicyRules]
R --> S[RenderPolicyYAML]
S --> T[policy_yaml_ConfigMap]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
RenderPolicyYAML, the comment says "Empty map returns empty YAML ("{}")" but the function actually returns"{}\n"; consider aligning the comment (and any reference docs) with the actual behavior to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `RenderPolicyYAML`, the comment says "Empty map returns empty YAML ("{}")" but the function actually returns `"{}\n"`; consider aligning the comment (and any reference docs) with the actual behavior to avoid confusion.
## Individual Comments
### Comment 1
<location path="internal/common/plugins/plugins.go" line_range="89" />
<code_context>
+}
+
+// insertToken inserts a token into the pipeline at the position specified.
+func insertToken(tokens []string, name string, pos types.PipelinePosition) []string {
+ if pos.After != "" {
+ for i, t := range tokens {
+ if t == pos.After {
+ result := make([]string, 0, len(tokens)+1)
+ result = append(result, tokens[:i+1]...)
+ result = append(result, name)
+ result = append(result, tokens[i+1:]...)
+ return result
+ }
+ }
+ }
+
+ if pos.Before != "" {
+ for i, t := range tokens {
+ if t == pos.Before {
+ result := make([]string, 0, len(tokens)+1)
+ result = append(result, tokens[:i]...)
+ result = append(result, name)
+ result = append(result, tokens[i:]...)
+ return result
+ }
+ }
+ }
+
+ // Default: append to end.
+ return append(tokens, name)
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** insertToken prioritizes After over Before and silently appends when no anchor is found, which might hide configuration mistakes
Consider either failing when a requested `Before`/`After` anchor is not found (so configuration errors surface early) or clearly documenting that invalid anchors fall back to an append so callers understand the behavior and its risks.
```suggestion
// insertToken inserts a token into the pipeline at the position specified.
// If both After and Before are set, After takes precedence.
// If the requested anchor token (After/Before) is not found in the existing
// pipeline, the token is appended to the end. This fallback behavior is
// intentional; callers that require strict placement should validate their
// configuration before invoking insertToken.
```
</issue_to_address>
### Comment 2
<location path="internal/common/conditions/conditions_test.go" line_range="159-148" />
<code_context>
+func TestGetCondition(t *testing.T) {
</code_context>
<issue_to_address>
**issue (testing):** Add a test that mutating the condition returned by GetCondition also updates the underlying slice element
`TestGetCondition` currently only checks the fields of the returned pointer, but not that it references the original slice element. Please also verify that mutating the returned condition (e.g. `c.Status = ...`) updates the original `[]metav1.Condition`, ensuring `GetCondition` returns a pointer into the slice rather than a copy, e.g.:
```go
g := NewGomegaWithT(t)
conditions := []metav1.Condition{{Type: "Ready", Status: metav1.ConditionFalse}}
c := GetCondition(conditions, "Ready")
g.Expect(c).NotTo(BeNil())
c.Status = metav1.ConditionTrue
g.Expect(conditions[0].Status).To(Equal(metav1.ConditionTrue))
```
</issue_to_address>
### Comment 3
<location path="internal/common/config/config_test.go" line_range="183" />
<code_context>
+ })
+}
+
+func TestInjectSecrets(t *testing.T) {
+ tests := []struct {
+ name string
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for incomplete or conflicting database secret inputs in InjectSecrets
The current `TestInjectSecrets` table is strong, but two edge cases are missing:
1. When only some of `database/user`, `database/password`, and `database/host` are set, `hasDatabaseParts` is false and `database.connection` is not produced. Please add a case that asserts this behavior so we don’t accidentally start emitting partial connection strings.
2. When both `database/connection` and the individual parts are provided, the code appears to prefer `database/connection`. A test that captures this precedence would help prevent regressions.
New table entries for these will better document and lock in the intended behavior for borderline configurations.
</issue_to_address>
### Comment 4
<location path="docs/reference/internal-common-packages.md" line_range="143" />
<code_context>
+| `config` | `map[string]map[string]string` | Current configuration to inject into |
+| `secrets` | `map[string]string` | Resolved secret values keyed by `section/key` |
+
+**Returns:** `map[string]map[string]string` — new config with secrets injected. Empty secrets is a no-op.
+
+**Database connection assembly:** When individual secret parts (`database/user`, `database/password`, `database/host`, and optionally `database/port`, `database/name`) are present, they are assembled into a connection string:
</code_context>
<issue_to_address>
**issue (typo):** Consider changing "Empty secrets is a no-op" to "Empty secrets are a no-op".
Because "secrets" is plural, use "are" instead of "is" in that sentence.
```suggestion
**Returns:** `map[string]map[string]string` — new config with secrets injected. Empty secrets are a no-op.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
issue (testing): Add a test that mutating the condition returned by GetCondition also updates the underlying slice element
TestGetCondition currently only checks the fields of the returned pointer, but not that it references the original slice element. Please also verify that mutating the returned condition (e.g. c.Status = ...) updates the original []metav1.Condition, ensuring GetCondition returns a pointer into the slice rather than a copy, e.g.:
g := NewGomegaWithT(t)
conditions := []metav1.Condition{{Type: "Ready", Status: metav1.ConditionFalse}}
c := GetCondition(conditions, "Ready")
g.Expect(c).NotTo(BeNil())
c.Status = metav1.ConditionTrue
g.Expect(conditions[0].Status).To(Equal(metav1.ConditionTrue))| }) | ||
| } | ||
|
|
||
| func TestInjectSecrets(t *testing.T) { |
There was a problem hiding this comment.
suggestion (testing): Add tests for incomplete or conflicting database secret inputs in InjectSecrets
The current TestInjectSecrets table is strong, but two edge cases are missing:
- When only some of
database/user,database/password, anddatabase/hostare set,hasDatabasePartsis false anddatabase.connectionis not produced. Please add a case that asserts this behavior so we don’t accidentally start emitting partial connection strings. - When both
database/connectionand the individual parts are provided, the code appears to preferdatabase/connection. A test that captures this precedence would help prevent regressions.
New table entries for these will better document and lock in the intended behavior for borderline configurations.
- Add shared type definitions (ImageSpec, DatabaseSpec, MessagingSpec, CacheSpec, PolicySpec, PluginSpec, PipelineSpec) for reuse across OpenStack operator CRDs - Add conditions package for managing K8s status conditions with LastTransitionTime preservation on no-op updates - Add config package with deterministic INI rendering, user-wins merge semantics, secret injection with RFC 3986 password encoding, and oslo_policy config injection - Add plugins package for generating PasteDeploy api-paste.ini pipelines with positional middleware insertion and plugin config rendering - Add policy package for merging oslo.policy rules with inline-wins precedence, validation via field.ErrorList, and deterministic YAML rendering - Include comprehensive table-driven tests with Gomega for all packages, covering edge cases (nil inputs, empty maps, mutation safety, idempotency) - Add reference documentation for internal/common packages and types AI-assisted: Claude Code On-behalf-of: @SAP christian.berendt@sap.com Signed-off-by: Christian Berendt <berendt@23technologies.cloud>
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
1 participant
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.
ImageSpec, DatabaseSpec, MessagingSpec, CacheSpec, PolicySpec,
PluginSpec, PipelineSpec, and related structs without kubebuilder
markers (deferred to CRD embedding sites)
and AllTrue helpers for managing K8s status conditions with
LastTransitionTime preservation on unchanged status
MergeDefaults, InjectSecrets with MySQL connection string assembly
and RFC 3986 password encoding, and InjectOsloPolicyConfig
api-paste.ini with positional middleware insertion and
RenderPluginConfig for converting plugin specs to config maps
ValidatePolicyRules returning field.ErrorList for webhooks, and
RenderPolicyYAML with deterministic sorted key output
covering edge cases, nil safety, and deterministic output