vcluster platform config restructure#3433
Conversation
6e79f78 to
e591520
Compare
7e38cf3 to
4017d06
Compare
b28cdd3 to
7655daa
Compare
f54ad1c to
c2f8c9c
Compare
There was a problem hiding this comment.
💡 Codex Review
Lines 1014 to 1019 in c2f8c9c
After the platform config moved to top-level fields, IsProFeatureEnabled no longer accounts for platform/sleep/snapshots/deletion. addVCluster (in create_helm/create_docker) relies on this flag to decide whether to hard-fail when the user isn’t logged in and no API key secret is provided. With the current logic, a user can set platform.project (or sleep/snapshot/deletion) while not logged in; InitClientFromConfig fails, IsProFeatureEnabled returns false, and the CLI silently skips creating the platform secret, leaving the cluster unregistered and the platform-only features inactive. Consider treating the new platform-related fields as pro features or explicitly erroring when they’re set but no login/secret is available.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
cb04c59 to
5634acc
Compare
Code reviewI found 1 issue during code review: Nil pointer dereference in IsConfiguredForSleepModeFile: config/config.go:933-939 The guard condition in Current code: func (c *Config) IsConfiguredForSleepMode() bool {
if c \!= nil && c.External \!= nil && c.External["platform"] == nil {
return false
}
return c.External["platform"]["autoSleep"] \!= nil || c.External["platform"]["autoDelete"] \!= nil
}Issue: The guard logic uses AND when it should use OR. When Suggested fix: func (c *Config) IsConfiguredForSleepMode() bool {
if c == nil || c.External == nil || c.External["platform"] == nil {
return false
}
return c.External["platform"]["autoSleep"] \!= nil || c.External["platform"]["autoDelete"] \!= nil
}This matches the correct pattern used in Lines 853 to 858 in 5634acc Checked for bugs and CLAUDE.md compliance. |
Depends on https://github.com/loft-sh/loft-enterprise/pull/5745
TODOs after merge of https://github.com/loft-sh/loft-enterprise/pull/5745
github.com/loft-sh/api/v4andgithub.com/loft-sh/agentapi/v4TODOs after merge of this PR
./hack/generate-typescript-client.shin platform in order to get schema for vclustermain.What issue type does this pull request address? (keep at least one, remove the others)
/kind bugfix
/kind enhancement
/kind feature
/kind documentation
/kind test
What does this pull request do? Which issues does it resolve? (use
resolves #<issue_number>if possible)Part of ENG-10404
Part of ENG-10495
Please provide a short message that should be published in the vcluster release notes
Fixed an issue where vcluster ...
What else do we need to know?