chore: refactored probe building and cluster defaults management#1916
chore: refactored probe building and cluster defaults management#1916
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 98 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="api/operator/v1beta1/vmextra_types.go">
<violation number="1" location="api/operator/v1beta1/vmextra_types.go:1229">
P3: Missing preposition "to" in GoDoc comments for `LivenessProbe` and `ReadinessProbe`. The `StartupProbe` comment correctly uses "added to CR pod", but these two say "added CR pod".
(Based on your team's feedback about documenting exported structs and public methods.) [FEEDBACK_USED]</violation>
</file>
<file name="api/operator/v1/vmanomaly_types.go">
<violation number="1" location="api/operator/v1/vmanomaly_types.go:458">
P3: Incorrect interface referenced in comment</violation>
</file>
<file name="internal/config/config.go">
<violation number="1" location="internal/config/config.go:809">
P1: Recursive env expansion in mapper can cause infinite recursion/stack overflow for cyclic variable references.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if val == "" { | ||
| val = opts.Environment[v] | ||
| } | ||
| return os.Expand(val, mapper) |
There was a problem hiding this comment.
P1: Recursive env expansion in mapper can cause infinite recursion/stack overflow for cyclic variable references.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/config/config.go, line 809:
<comment>Recursive env expansion in mapper can cause infinite recursion/stack overflow for cyclic variable references.</comment>
<file context>
@@ -767,8 +799,14 @@ func (labels *Labels) Set(value string) error {
+ if val == "" {
+ val = opts.Environment[v]
+ }
+ return os.Expand(val, mapper)
}
params, err := env.GetFieldParamsWithOptions(cfg, opts)
</file context>
AndrewChubatiuk
force-pushed
the
refactor-probes-and-defaults
branch
2 times, most recently
from
8932c2d to
5491f58
Compare
| opts := getEnvOpts() | ||
| mapper := func(v string) string { | ||
| return opts.Environment[v] | ||
| rawEnvVars := make(map[string]string) |
There was a problem hiding this comment.
This is a dangerous function - we'll be expanding all env vars, not limited to the env var we want.
Lets move this expansion to UseProxyProtocol?
There was a problem hiding this comment.
it's a subset of functionality from carlos0/env library, which we're using already and this function is only used once to init metrics with env variables names and values
this expansion has nothing to do with useProxyProtocol
There was a problem hiding this comment.
Oh, I see - its used in declaration like VM_OPERATOR_VERSION,expand. Why would we want to change anything here - is it for recursive env var expansion? In that case its best to limit the depth to some sensible value - 3 or maybe even 5
There was a problem hiding this comment.
is it for recursive env var expansion?
yes
its best to limit the depth to some sensible value - 3 or maybe even 5
agree, will update soon
AndrewChubatiuk
force-pushed
the
refactor-probes-and-defaults
branch
from
3f7044d to
bf97d25
Compare
- temporary replaced caarlos0/env with fork till PR is merged - consolidate all functions for cluster defaults population - merged EmbeddedProbes, CommonDefaultableParams and CommonApplicationDeploymentParams into CommonAppsParams - moved setting terminationGracePeriodSeconds to defaults management
Co-authored-by: Vadim Rutkovsky <vadim@vrutkovs.eu> Signed-off-by: Andrii Chubatiuk <andrew.chubatiuk@gmail.com>
AndrewChubatiuk
force-pushed
the
refactor-probes-and-defaults
branch
from
bf97d25 to
3de2008
Compare
Co-authored-by: Vadim Rutkovsky <vadim@vrutkovs.eu>
Co-authored-by: Vadim Rutkovsky <vadim@vrutkovs.eu>
2 participants
Summary by cubic
Refactored probe handling and app/cluster defaults to simplify CRDs and centralize config. Probes are now built from CommonAppsParams, support proxy protocol via extraArgs, and termination grace is controlled by operator defaults per app.
Refactors
Migration
Written for commit 3de2008. Summary will update on new commits.