Repository navigation
Conversation
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
🔒 Vulnerability Check Results
Showing 4 of 7 total findings. govulncheck exit code: 0 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 24 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Signed-off-by: Vad1mo <vadim@8gears.com>
Vad1mo
force-pushed
the
feat/optional-features
branch
from
November 5, 2026 15:06
0c92975 to
c7c9a5a
Compare
Contributor
|
Preview images for this PR are available in
Verify a preview image: Verify the SBOM attestation: |
- keep the beta badge and info icon from toggling the feature - keep Save, Cancel and toggles disabled until the config reload finishes - run every disable guard registered for a feature instead of the last one - refuse a registry type claimed by two features Signed-off-by: Vad1mo <vadim@8gears.com>
There was a problem hiding this comment.
8 issues found across 24 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/lib/config/metadata/metadata_test.go">
<violation number="1" location="src/lib/config/metadata/metadata_test.go:54">
P2: This test permanently mutates the package-global `ConfigList`, so repeated test runs in one process fail when the synthetic key is already registered. Snapshot `ConfigList` and restore it with `t.Cleanup`, then reinitialize the metadata singleton.</violation>
</file>
<file name="src/controller/systeminfo/controller.go">
<violation number="1" location="src/controller/systeminfo/controller.go:142">
P2: This breaks the existing `TestGetInfo` case: `enabledFeatures` always returns a non-nil empty map, but the test expects `Features` to be nil. Either set `Features: map[string]bool{}` in that expectation in `controller_test.go`, or return nil from `enabledFeatures` when there are no features.</violation>
</file>
<file name="src/controller/config/controller.go">
<violation number="1" location="src/controller/config/controller.go:98">
P2: `feature.NotifyChanged` runs only on the replica that handled the request. Other replicas pick up the new setting through the Postgres LISTEN/NOTIFY sync but never fire `OnChange` listeners. A listener that keeps per-process state will go stale on those replicas. Route the notification through the synced-settings refresh path, or document `OnChange` as single-instance, before the first listener is added.</violation>
<violation number="2" location="src/controller/config/controller.go:98">
P2: `NotifyChanged` runs before the config store publishes its update. Register it with `orm.AfterCommit` so listeners observe the committed value and are not notified for rolled-back transactions.</violation>
</file>
<file name="src/pkg/feature/feature.go">
<violation number="1" location="src/pkg/feature/feature.go:135">
P2: Custom agent: **Enforce Strict Maintainability Standards**
This compatibility path accepts an existing boolean item with the same environment key but a different `DefaultValue`, so the feature gate can fall back to `Definition.Default` while configuration metadata retains a conflicting default. Reject existing items whose default differs from the definition.</violation>
<violation number="2" location="src/pkg/feature/feature.go:318">
P2: The legacy env fallback only triggers when the new variable is absent. If `EnvKey` is set to an empty string, for example by a Helm or compose template, the feature is forced off and the legacy variable is ignored. Treat an empty value as unset before falling back to `LegacyEnvKey`.</violation>
</file>
<file name="api/v2.0/swagger.yaml">
<violation number="1" location="api/v2.0/swagger.yaml:342">
P3: `PUT /features/{feature_key}` returns 422 when the required `enabled` property is omitted, but this operation documents only 400/401/403/404/500. Add the shared 422 response so API clients can model validation failures.</violation>
</file>
<file name="src/portal/src/app/base/left-side-nav/config/features/features.component.ts">
<violation number="1" location="src/portal/src/app/base/left-side-nav/config/features/features.component.ts:104">
P2: Custom agent: **Enforce Strict Maintainability Standards**
Saving multiple toggles now sends one request per key, so earlier settings can be persisted while later ones are skipped when an update fails. Send the pending changes through one bulk update so the save is validated and applied as a unit.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| func TestRegisterAfterInstanceIsBuilt(t *testing.T) { | ||
| Instance().init() | ||
| item := Item{Name: "registered_after_init", Scope: UserScope, Group: BasicGroup, DefaultValue: "true", ItemType: &BoolType{}, Editable: true} |
There was a problem hiding this comment.
P2: This test permanently mutates the package-global ConfigList, so repeated test runs in one process fail when the synthetic key is already registered. Snapshot ConfigList and restore it with t.Cleanup, then reinitialize the metadata singleton.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/lib/config/metadata/metadata_test.go, line 54:
<comment>This test permanently mutates the package-global `ConfigList`, so repeated test runs in one process fail when the synthetic key is already registered. Snapshot `ConfigList` and restore it with `t.Cleanup`, then reinitialize the metadata singleton.</comment>
<file context>
@@ -48,3 +48,23 @@ func TestCfgMetaData_Init(t *testing.T) {
+
+func TestRegisterAfterInstanceIsBuilt(t *testing.T) {
+ Instance().init()
+ item := Item{Name: "registered_after_init", Scope: UserScope, Group: BasicGroup, DefaultValue: "true", ItemType: &BoolType{}, Editable: true}
+
+ got, added := Register(item)
</file context>
| ProjectCreationRestrict: utils.SafeCastString(cfg[common.ProjectCreationRestriction]), | ||
| RegistryStorageProviderName: utils.SafeCastString(cfg[common.RegistryStorageProviderName]), | ||
| NotificationEnable: utils.SafeCastBool(cfg[common.NotificationEnable]), | ||
| Features: enabledFeatures(ctx), |
There was a problem hiding this comment.
P2: This breaks the existing TestGetInfo case: enabledFeatures always returns a non-nil empty map, but the test expects Features to be nil. Either set Features: map[string]bool{} in that expectation in controller_test.go, or return nil from enabledFeatures when there are no features.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/controller/systeminfo/controller.go, line 142:
<comment>This breaks the existing `TestGetInfo` case: `enabledFeatures` always returns a non-nil empty map, but the test expects `Features` to be nil. Either set `Features: map[string]bool{}` in that expectation in `controller_test.go`, or return nil from `enabledFeatures` when there are no features.</comment>
<file context>
@@ -137,10 +139,20 @@ func (c *controller) GetInfo(ctx context.Context, opt Options) (*Data, error) {
ProjectCreationRestrict: utils.SafeCastString(cfg[common.ProjectCreationRestriction]),
RegistryStorageProviderName: utils.SafeCastString(cfg[common.RegistryStorageProviderName]),
NotificationEnable: utils.SafeCastBool(cfg[common.NotificationEnable]),
+ Features: enabledFeatures(ctx),
}
return res, nil
</file context>
| log.Errorf("failed to upload configurations: %v", err) | ||
| return fmt.Errorf("failed to validate configuration") | ||
| } | ||
| feature.NotifyChanged(conf) |
There was a problem hiding this comment.
P2: feature.NotifyChanged runs only on the replica that handled the request. Other replicas pick up the new setting through the Postgres LISTEN/NOTIFY sync but never fire OnChange listeners. A listener that keeps per-process state will go stale on those replicas. Route the notification through the synced-settings refresh path, or document OnChange as single-instance, before the first listener is added.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/controller/config/controller.go, line 98:
<comment>`feature.NotifyChanged` runs only on the replica that handled the request. Other replicas pick up the new setting through the Postgres LISTEN/NOTIFY sync but never fire `OnChange` listeners. A listener that keeps per-process state will go stale on those replicas. Route the notification through the synced-settings refresh path, or document `OnChange` as single-instance, before the first listener is added.</comment>
<file context>
@@ -95,6 +95,7 @@ func (c *controller) UpdateUserConfigs(ctx context.Context, conf map[string]any)
log.Errorf("failed to upload configurations: %v", err)
return fmt.Errorf("failed to validate configuration")
}
+ feature.NotifyChanged(conf)
// update the audit logger to point to the new endpoint
return c.updateLogEndpoint(ctx, conf)
</file context>
| func environmentOverride(definition Definition) (enabled bool, envKey string, overridden bool) { | ||
| envKey = definition.EnvKey | ||
| value, exists := os.LookupEnv(envKey) | ||
| if !exists && definition.LegacyEnvKey != "" { |
There was a problem hiding this comment.
P2: The legacy env fallback only triggers when the new variable is absent. If EnvKey is set to an empty string, for example by a Helm or compose template, the feature is forced off and the legacy variable is ignored. Treat an empty value as unset before falling back to LegacyEnvKey.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/pkg/feature/feature.go, line 318:
<comment>The legacy env fallback only triggers when the new variable is absent. If `EnvKey` is set to an empty string, for example by a Helm or compose template, the feature is forced off and the legacy variable is ignored. Treat an empty value as unset before falling back to `LegacyEnvKey`.</comment>
<file context>
@@ -133,22 +300,37 @@ func ConfigOverride(configKey string) (enabled bool, overridden bool) {
- value, exists := os.LookupEnv(definition.EnvKey)
+ envKey = definition.EnvKey
+ value, exists := os.LookupEnv(envKey)
+ if !exists && definition.LegacyEnvKey != "" {
+ envKey = definition.LegacyEnvKey
+ value, exists = os.LookupEnv(envKey)
</file context>
Suggested change
| if !exists && definition.LegacyEnvKey != "" { | |
| if value == "" && definition.LegacyEnvKey != "" { |
| log.Errorf("failed to upload configurations: %v", err) | ||
| return fmt.Errorf("failed to validate configuration") | ||
| } | ||
| feature.NotifyChanged(conf) |
There was a problem hiding this comment.
P2: NotifyChanged runs before the config store publishes its update. Register it with orm.AfterCommit so listeners observe the committed value and are not notified for rolled-back transactions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/controller/config/controller.go, line 98:
<comment>`NotifyChanged` runs before the config store publishes its update. Register it with `orm.AfterCommit` so listeners observe the committed value and are not notified for rolled-back transactions.</comment>
<file context>
@@ -95,6 +95,7 @@ func (c *controller) UpdateUserConfigs(ctx context.Context, conf map[string]any)
log.Errorf("failed to upload configurations: %v", err)
return fmt.Errorf("failed to validate configuration")
}
+ feature.NotifyChanged(conf)
// update the audit logger to point to the new endpoint
return c.updateLogEndpoint(ctx, conf)
</file context>
| this.onGoing = true; | ||
| // Sequential, so a rejected change (env lock, disable guard) stops | ||
| // the rest and the reload shows what was actually stored. | ||
| concat( |
There was a problem hiding this comment.
P2: Custom agent: Enforce Strict Maintainability Standards
Saving multiple toggles now sends one request per key, so earlier settings can be persisted while later ones are skipped when an update fails. Send the pending changes through one bulk update so the save is validated and applied as a unit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/portal/src/app/base/left-side-nav/config/features/features.component.ts, line 104:
<comment>Saving multiple toggles now sends one request per key, so earlier settings can be persisted while later ones are skipped when an update fails. Send the pending changes through one bulk update so the save is validated and applied as a unit.</comment>
<file context>
@@ -12,109 +12,126 @@
- });
+ // Sequential, so a rejected change (env lock, disable guard) stops
+ // the rest and the reload shows what was actually stored.
+ concat(
+ ...keys.map(key =>
+ this.configureService.updateFeature({
</file context>
| if added { | ||
| return | ||
| } | ||
| if _, ok := item.ItemType.(*metadata.BoolType); !ok || item.EnvKey != def.EnvKey { |
There was a problem hiding this comment.
P2: Custom agent: Enforce Strict Maintainability Standards
This compatibility path accepts an existing boolean item with the same environment key but a different DefaultValue, so the feature gate can fall back to Definition.Default while configuration metadata retains a conflicting default. Reject existing items whose default differs from the definition.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/pkg/feature/feature.go, line 135:
<comment>This compatibility path accepts an existing boolean item with the same environment key but a different `DefaultValue`, so the feature gate can fall back to `Definition.Default` while configuration metadata retains a conflicting default. Reject existing items whose default differs from the definition.</comment>
<file context>
@@ -12,64 +12,173 @@
+ if added {
+ return
+ }
+ if _, ok := item.ItemType.(*metadata.BoolType); !ok || item.EnvKey != def.EnvKey {
+ panic(fmt.Sprintf("config item %q exists with a different type or env key", def.ConfigKey))
+ }
</file context>
Comment on lines
+342
to
+343
| '404': | ||
| $ref: '#/responses/404' |
There was a problem hiding this comment.
P3: PUT /features/{feature_key} returns 422 when the required enabled property is omitted, but this operation documents only 400/401/403/404/500. Add the shared 422 response so API clients can model validation failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At api/v2.0/swagger.yaml, line 342:
<comment>`PUT /features/{feature_key}` returns 422 when the required `enabled` property is omitted, but this operation documents only 400/401/403/404/500. Add the shared 422 response so API clients can model validation failures.</comment>
<file context>
@@ -289,6 +289,60 @@ paths:
+ $ref: '#/responses/401'
+ '403':
+ $ref: '#/responses/403'
+ '404':
+ $ref: '#/responses/404'
+ '500':
</file context>
Suggested change
| '404': | |
| $ref: '#/responses/404' | |
| '404': | |
| $ref: '#/responses/404' | |
| '422': | |
| $ref: '#/responses/422' |
…res API
feature.Register now carries name, description, section, beta and default
and creates the boolean config item itself (metadata.Register works after
the config managers froze the item list). The Features page renders from
GET /api/v2.0/features and saves through PUT /api/v2.0/features/{key};
systeminfo reports a features map for UI gating. Adding a feature no
longer needs swagger fields, a metadata file or a portal entry.
RegisterSection adds page sections with a link resolved per request.
Resolve and OnChange expose effective values and change events, so an
OpenFeature provider can be added later without touching call sites.
Signed-off-by: Vad1mo <vadim@8gears.com>
Signed-off-by: Vad1mo <vadim@8gears.com>
Vad1mo
force-pushed
the
feat/optional-features
branch
from
November 5, 2026 23:05
d25bafb to
9eba68d
Compare
Signed-off-by: Vad1mo <vadim@8gears.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/server/v2.0/handler/feature.go">
<violation number="1" location="src/server/v2.0/handler/feature.go:63">
P3: The name-only comparator leaves features with the same display name in registration order, which the comment above says should not matter. Add ConfigKey as a tie-breaker so the list is fully deterministic.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Registration order follows Go package initialization, which users | ||
| // should not see; sort by display name for a stable list. | ||
| definitions := feature.Definitions() | ||
| sort.SliceStable(definitions, func(i, j int) bool { |
There was a problem hiding this comment.
P3: The name-only comparator leaves features with the same display name in registration order, which the comment above says should not matter. Add ConfigKey as a tie-breaker so the list is fully deterministic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/server/v2.0/handler/feature.go, line 63:
<comment>The name-only comparator leaves features with the same display name in registration order, which the comment above says should not matter. Add ConfigKey as a tie-breaker so the list is fully deterministic.</comment>
<file context>
@@ -55,7 +57,12 @@ func (c *configAPI) UpdateFeature(ctx context.Context, params configure.UpdateFe
+ // Registration order follows Go package initialization, which users
+ // should not see; sort by display name for a stable list.
definitions := feature.Definitions()
+ sort.SliceStable(definitions, func(i, j int) bool {
+ return strings.ToLower(definitions[i].Name) < strings.ToLower(definitions[j].Name)
+ })
</file context>
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
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.
Adds a generic on/off gate for optional features and a Configuration > Features page that lists them. No feature is registered yet, so the tab stays hidden on a plain build. #1398 (Hugging Face model import) is the first planned user.
What it contains
src/pkg/feature: onefeature.Register(Definition{...})call per feature carries config key, env key (plus optional legacy name), section, name, description, beta and default. It creates the boolean config item itself;metadata.Registermakes that possible after the config managers froze the item list.RegisterSectionadds page sections; a section link is resolved per request, so it can carry the current stateGET /api/v2.0/features(sections and features withenabled,editable,locked_by) andPUT /api/v2.0/features/{key}, which goes through the config controller (env lock, guards,CONFIG_OVERWRITE_JSON).GET /systeminfoadds afeaturesmap for UI gating.Enabled(ctx, ...)keeps the context, andResolve/OnChangeare exported so a provider can be added in its own package later.Adding a feature (as done in #1398)
That is the whole toggle: no swagger field, no metadata file, no portal entry. Gate code paths with
feature.Enabled(ctx, f), a registry type withfeature.RegisterRegistryTypes, UI withAppConfigService.featureEnabled(key).Verified
pkg/feature,lib/config/...,pkg/config/...,controller/config,server/v2.0/handler(incl. API tests for 401/403/404/422/400) with-race; golangci-lint v2.15.0: 0 issuesleft-side-nav/config/**: 45/45;tscon the whole portal cleanfalseis serialized, env lock andPUTerrors as aboveScreenshot
Example: #1398 (Hugging Face model import) registered as the first optional feature, on top of this PR (branch
feat/hf-model-import-option-b@ 99f1cd6). Dev stack on slot 0, toggle switched on and saved.Release Notes
Configuration has a new Features tab for switching optional features on and off, also available as
GET/PUT /api/v2.0/features. An environment variableHARBOR_ENABLE_<FEATURE>takes precedence and locks the setting in the UI. The tab appears once a release ships an optional feature.