OCPBUGS-79666: add pull secret auto populate logic to ABI local UI#3508
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@ElayAharoni: This pull request references Jira Issue OCPBUGS-79666, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: bmanzari. Note that only openshift-assisted members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
bot
added
the
size/XXL
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds a dedicated HTTP GET endpoint at Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClusterDetails as ClusterDetails Component
participant usePullSecret as usePullSecret Hook
participant BridgeAPI as Bridge API<br/>(/api/pull-secret)
participant OCM as OCM API
User->>ClusterDetails: Mount Component
ClusterDetails->>usePullSecret: Call usePullSecret(isSingleClusterFeatureEnabled)
alt isInOcm is true
usePullSecret->>OCM: fetchPullSecret()
OCM-->>usePullSecret: Pull secret config
usePullSecret-->>ClusterDetails: Pull secret value
else isInOcm is false AND isSingleClusterFeatureEnabled
usePullSecret->>BridgeAPI: GET /api/pull-secret
BridgeAPI-->>usePullSecret: Manifest file contents
usePullSecret-->>ClusterDetails: Trimmed pull secret
else isInOcm is false AND NOT isSingleClusterFeatureEnabled
usePullSecret-->>ClusterDetails: No update (uses existing)
end
ClusterDetails->>User: Render with pull secret value
sequenceDiagram
participant User
participant PullSecret as PullSecret Component
participant FormField as Form Field
participant Backend as Backend Handler
User->>PullSecret: Render Component<br/>(with isPullSecretSet)
alt isPullSecretSet is true
PullSecret->>PullSecret: Display success alert
end
PullSecret->>PullSecret: Show PullSecretField<br/>(checkbox or direct)
PullSecret->>FormField: Bind to Formik field
User->>PullSecret: Check/Edit pull secret
PullSecret->>FormField: Update value
FormField-->>User: Render updated state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
|
@ElayAharoni: This pull request references Jira Issue OCPBUGS-79666, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: bmanzari. Note that only openshift-assisted members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-lib/lib/ocm/hooks/usePullSecret.ts (1)
51-55:⚠️ Potential issue | 🟡 MinorPotential stale closure bug due to missing dependency.
The
useEffecthas an empty dependency array but callsgetPullSecret, which depends onisSingleClusterFeatureEnabled. IfisSingleClusterFeatureEnabledchanges after initial render,getPullSecretwon't be called again with the updated value.The ESLint disable comment suggests this was intentional, but the current logic could lead to:
- Initial render with
isSingleClusterFeatureEnabled = undefined→ no bridge fetch- If the value later becomes
true, the effect won't re-runConsider adding getPullSecret to the dependency array
React.useEffect(() => { if (!pullSecret) { void getPullSecret(); } - }, []); // eslint-disable-line react-hooks/exhaustive-deps + }, [getPullSecret, pullSecret]);Or, if the single-run behavior is intentional, document why:
- }, []); // eslint-disable-line react-hooks/exhaustive-deps + }, []); // eslint-disable-line react-hooks/exhaustive-deps -- intentionally run once on mount only🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-lib/lib/ocm/hooks/usePullSecret.ts` around lines 51 - 55, The effect in usePullSecret calls getPullSecret but has an empty dependency array, causing a stale closure when isSingleClusterFeatureEnabled changes; update the React.useEffect dependencies to include getPullSecret (or isSingleClusterFeatureEnabled and pullSecret) so the effect re-runs when feature flag changes, or alternatively document and keep the single-run behavior if intentional; locate the React.useEffect block that references getPullSecret, pullSecret, and isSingleClusterFeatureEnabled and either add getPullSecret to the dependency array or add a clear comment explaining why the effect must run only once.
🧹 Nitpick comments (3)
apps/assisted-disconnected-ui/proxy/pullsecret/handler.go (2)
27-30: Method check is redundant given router configuration.The router in
app.goalready restricts this route toGETvia.Methods(http.MethodGet). The method check here is defensive but redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/assisted-disconnected-ui/proxy/pullsecret/handler.go` around lines 27 - 30, Remove the redundant HTTP method check block (the if r.Method != http.MethodGet { w.WriteHeader(http.StatusMethodNotAllowed); return }) from the pull secret handler so the route relies on the router's .Methods(http.MethodGet) restriction; locate and delete this conditional inside the handler function that receives (w, r) (e.g., the pull secret handler) and ensure no other logic depends on that early return.
34-37: Debug-level logging may obscure deployment issues.When the pull-secret manifest is missing, logging at
Debuglevel means this won't appear in typical production log configurations. If the deployment is misconfigured (e.g., the file isn't mounted at the expected path), operators may not notice the issue since the endpoint silently returns 404.Consider logging at
Warnlevel, at least on first occurrence, or adding a startup check that validates the configured path exists.Consider logging at a higher level for visibility
if os.IsNotExist(err) { - log.GetLog().WithField("path", manifestPath).Debug("pull secret manifest not found") + log.GetLog().WithField("path", manifestPath).Warn("pull secret manifest not found at configured path") w.WriteHeader(http.StatusNotFound) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/assisted-disconnected-ui/proxy/pullsecret/handler.go` around lines 34 - 37, The handler currently logs a missing pull-secret manifest at Debug level which can hide deployment issues; update the log call in the handler that checks os.IsNotExist(err) to use Warn (e.g., replace log.GetLog().WithField("path", manifestPath).Debug(...) with a Warn-level call) so missing file events are visible in production, and optionally add a startup validation function (run on init) that checks manifestPath existence and logs a clear error/warning if absent using the same logger (log.GetLog()) to catch misconfigured mounts early.apps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/apic.go (1)
128-130: Ensure complete writes in the YAML handler wrapper.Go's io.Writer contract guarantees that if
Writereturnsn < len(buffer), it must also return a non-nil error. However, if defensive full-write semantics are desired for this handler, loop until the buffer is fully drained rather than returning after a single (potentially partial) write.🛠️ Proposed fix
func yaml_writer_write_handler(emitter *yaml_emitter_t, buffer []byte) error { - _, err := emitter.output_writer.Write(buffer) - return err + for len(buffer) > 0 { + n, err := emitter.output_writer.Write(buffer) + if err != nil { + return err + } + if n == 0 { + return io.ErrShortWrite + } + buffer = buffer[n:] + } + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/apic.go` around lines 128 - 130, The yaml_writer_write_handler currently returns after a single Write call which may be partial; change it to repeatedly call emitter.output_writer.Write until the entire buffer is written or an error occurs. In other words, in yaml_writer_write_handler loop over the remaining slice (using the returned n to advance the offset) and only return nil when total bytes written equals len(buffer), otherwise return the encountered non-nil error from Write.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/assisted-disconnected-ui/proxy/app.go`:
- Around line 31-33: The /pull-secret route is unprotected: wrap the existing
router.HandleFunc("/pull-secret",
pullsecret.Handler(config.PullSecretManifestPath)) registration with the same
auth/authorization middleware used for other sensitive proxy endpoints (or apply
the same handler chain used by apiHandler), so requests to "/pull-secret" must
authenticate and pass authorization checks before calling pullsecret.Handler; on
failure return the appropriate 401/403 response and log the denied attempt.
Ensure you reuse the existing auth middleware function(s) (the same middleware
applied to apiHandler or other protected routes) rather than creating a
different policy, and keep pullsecret.Handler(config.PullSecretManifestPath) as
the final handler in the chain.
In `@apps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/yaml.go`:
- Around line 471-483: The Node.SetString method must clear any previous style
bits before recomputing so prior styles (e.g., DoubleQuotedStyle, TaggedStyle)
don't leak into reused nodes; modify Node.SetString to reset n.Style (e.g.,
n.Style = 0 or PlainStyle) at the start (or right after setting Tag/Value) and
then reapply LiteralStyle only if strings.Contains(n.Value, "\n") so Style
reflects the new content; key symbols: Node.SetString, n.Style, LiteralStyle.
In `@libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetails.tsx`:
- Line 49: Restore infraEnv.pullSecret as the highest-precedence source for
single-cluster edit flows by changing how pullSecret is computed: pick
infraEnv?.pullSecret first and only fall back to the hook value from
usePullSecret(isSingleClusterFeatureEnabled) when infraEnv.pullSecret is
undefined/null. Update the assignment in ClusterDetails (the line using
usePullSecret) so it uses infraEnv?.pullSecret ??
usePullSecret(isSingleClusterFeatureEnabled) (or equivalent) to preserve
existing infra-env pull secrets.
In `@libs/ui-lib/lib/ocm/hooks/usePullSecret.ts`:
- Around line 44-47: The alert title is a user-facing string and must be
translated: in usePullSecret.ts inside the handleApiError callback where
addAlert({ title: 'Failed to retrieve pull secret', ... }) is called, wrap the
title with t('ai:Failed to retrieve pull secret') and ensure the t translation
function is imported (or available) in the module; keep the rest of the callback
(setPullSecret and message: getApiErrorMessage(e)) unchanged.
---
Outside diff comments:
In `@libs/ui-lib/lib/ocm/hooks/usePullSecret.ts`:
- Around line 51-55: The effect in usePullSecret calls getPullSecret but has an
empty dependency array, causing a stale closure when
isSingleClusterFeatureEnabled changes; update the React.useEffect dependencies
to include getPullSecret (or isSingleClusterFeatureEnabled and pullSecret) so
the effect re-runs when feature flag changes, or alternatively document and keep
the single-run behavior if intentional; locate the React.useEffect block that
references getPullSecret, pullSecret, and isSingleClusterFeatureEnabled and
either add getPullSecret to the dependency array or add a clear comment
explaining why the effect must run only once.
---
Nitpick comments:
In `@apps/assisted-disconnected-ui/proxy/pullsecret/handler.go`:
- Around line 27-30: Remove the redundant HTTP method check block (the if
r.Method != http.MethodGet { w.WriteHeader(http.StatusMethodNotAllowed); return
}) from the pull secret handler so the route relies on the router's
.Methods(http.MethodGet) restriction; locate and delete this conditional inside
the handler function that receives (w, r) (e.g., the pull secret handler) and
ensure no other logic depends on that early return.
- Around line 34-37: The handler currently logs a missing pull-secret manifest
at Debug level which can hide deployment issues; update the log call in the
handler that checks os.IsNotExist(err) to use Warn (e.g., replace
log.GetLog().WithField("path", manifestPath).Debug(...) with a Warn-level call)
so missing file events are visible in production, and optionally add a startup
validation function (run on init) that checks manifestPath existence and logs a
clear error/warning if absent using the same logger (log.GetLog()) to catch
misconfigured mounts early.
In `@apps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/apic.go`:
- Around line 128-130: The yaml_writer_write_handler currently returns after a
single Write call which may be partial; change it to repeatedly call
emitter.output_writer.Write until the entire buffer is written or an error
occurs. In other words, in yaml_writer_write_handler loop over the remaining
slice (using the returned n to advance the offset) and only return nil when
total bytes written equals len(buffer), otherwise return the encountered non-nil
error from Write.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d90579b-8b4f-467d-b807-b071563da099
⛔ Files ignored due to path filters (1)
apps/assisted-disconnected-ui/proxy/go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
apps/assisted-disconnected-ui/proxy/app.goapps/assisted-disconnected-ui/proxy/config/config.goapps/assisted-disconnected-ui/proxy/go.modapps/assisted-disconnected-ui/proxy/pullsecret/handler.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/LICENSEapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/NOTICEapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/README.mdapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/apic.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/decode.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/emitterc.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/encode.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/parserc.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/readerc.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/resolve.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/scannerc.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/sorter.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/writerc.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/yaml.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/yamlh.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/yamlprivateh.goapps/assisted-disconnected-ui/proxy/vendor/modules.txtapps/assisted-disconnected-ui/vite.config.tslibs/ui-lib/lib/common/components/clusters/PullSecret.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/OcmClusterDetailsFormFields.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetails.tsxlibs/ui-lib/lib/ocm/hooks/usePullSecret.ts
ElayAharoni
force-pushed
the
pull-secret-auto-populated-in-ABI-local-UI
branch
2 times, most recently
from
b9d5a91 to
7d92954
Compare
openshift-ci
bot
added
size/L
openshift-ci
bot
added
lgtm
|
/approve |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetails.tsx (1)
49-49:⚠️ Potential issue | 🟠 MajorPreserve
infraEnv.pullSecretprecedence in single-cluster edit flows.The hook in
libs/ui-lib/lib/ocm/hooks/usePullSecret.tsstill only pulls from OCM/bridge, so an existinginfraEnv.pullSecretnever reachesClusterDetailsForm. In single-cluster edit flows that can replace the saved secret with the fallback value instead of preserving the one already attached to the infra-env.Proposed patch
- const pullSecret = usePullSecret(isSingleClusterFeatureEnabled); + const fetchedPullSecret = usePullSecret(isSingleClusterFeatureEnabled); + const pullSecret = isSingleClusterFeatureEnabled + ? infraEnv?.pullSecret ?? fetchedPullSecret + : fetchedPullSecret;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetails.tsx` at line 49, The current call in ClusterDetails.tsx uses usePullSecret(isSingleClusterFeatureEnabled) which ignores an existing infraEnv.pullSecret and causes single-cluster edit flows to lose the saved secret; update the code so the infraEnv.pullSecret takes precedence: either extend the hook usePullSecret to accept an optional existingPullSecret parameter and return that when present, or change ClusterDetails.tsx to compute const pullSecret = infraEnv?.pullSecret ?? usePullSecret(isSingleClusterFeatureEnabled) and pass that pullSecret into ClusterDetailsForm; reference the usePullSecret hook, the ClusterDetails component, ClusterDetailsForm, and the infraEnv.pullSecret property when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/assisted-disconnected-ui/proxy/pullsecret/handler.go`:
- Around line 25-46: The Handler function currently serves the pull-secret
manifest without any authentication; fix this by enforcing the existing API auth
boundary: either register Handler through the existing apiHandler/middleware
chain (so it inherits the API auth) or add an explicit auth check at the top of
Handler (call the same authentication/authorization routine your app uses and
return http.StatusUnauthorized / http.StatusForbidden on failure) before reading
manifestPath; keep the same behavior for GET-only requests and return the
manifest only after successful auth.
---
Duplicate comments:
In `@libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetails.tsx`:
- Line 49: The current call in ClusterDetails.tsx uses
usePullSecret(isSingleClusterFeatureEnabled) which ignores an existing
infraEnv.pullSecret and causes single-cluster edit flows to lose the saved
secret; update the code so the infraEnv.pullSecret takes precedence: either
extend the hook usePullSecret to accept an optional existingPullSecret parameter
and return that when present, or change ClusterDetails.tsx to compute const
pullSecret = infraEnv?.pullSecret ??
usePullSecret(isSingleClusterFeatureEnabled) and pass that pullSecret into
ClusterDetailsForm; reference the usePullSecret hook, the ClusterDetails
component, ClusterDetailsForm, and the infraEnv.pullSecret property when making
the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 502b8c52-6e44-4aa7-93ef-b40e51f422e7
⛔ Files ignored due to path filters (1)
apps/assisted-disconnected-ui/proxy/go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
apps/assisted-disconnected-ui/proxy/app.goapps/assisted-disconnected-ui/proxy/config/config.goapps/assisted-disconnected-ui/proxy/go.modapps/assisted-disconnected-ui/proxy/pullsecret/handler.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/LICENSEapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/NOTICEapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/README.mdapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/apic.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/decode.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/emitterc.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/encode.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/parserc.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/readerc.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/resolve.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/scannerc.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/sorter.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/writerc.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/yaml.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/yamlh.goapps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/yamlprivateh.goapps/assisted-disconnected-ui/proxy/vendor/modules.txtlibs/locales/lib/en/translation.jsonlibs/ui-lib/lib/common/components/clusters/PullSecret.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/OcmClusterDetailsFormFields.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetails.tsxlibs/ui-lib/lib/ocm/hooks/usePullSecret.ts
✅ Files skipped from review due to trivial changes (8)
- apps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/NOTICE
- apps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/LICENSE
- apps/assisted-disconnected-ui/proxy/go.mod
- libs/locales/lib/en/translation.json
- apps/assisted-disconnected-ui/proxy/config/config.go
- apps/assisted-disconnected-ui/proxy/vendor/modules.txt
- apps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/yamlprivateh.go
- apps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/readerc.go
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/assisted-disconnected-ui/proxy/app.go
- libs/ui-lib/lib/ocm/hooks/usePullSecret.ts
- apps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/writerc.go
- apps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/encode.go
- apps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/decode.go
- apps/assisted-disconnected-ui/proxy/vendor/gopkg.in/yaml.v3/yamlh.go
ElayAharoni
force-pushed
the
pull-secret-auto-populated-in-ABI-local-UI
branch
from
7d92954 to
1d06dbc
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
libs/ui-lib/lib/common/components/clusters/PullSecret.tsx (1)
11-12:⚠️ Potential issue | 🟠 MajorMake
isSingleClusterFeatureEnabledexplicit (non-optional) to prevent silent behavior misses.Line 33 now changes rendering based on
isSingleClusterFeatureEnabled, but Line 12/Line 19 makes it optional with afalsedefault. Callers that don’t pass it silently fall back to the bare-field path, which can skip the new ABI-local edit flow.
Given this component API change, require the prop and remove the default to force explicit call-site decisions.Proposed API hardening diff
export type PullSecretProps = { defaultPullSecret?: string; isOcm?: boolean; isPullSecretSet?: boolean; - isSingleClusterFeatureEnabled?: boolean; + isSingleClusterFeatureEnabled: boolean; }; const PullSecret: React.FC<PullSecretProps> = ({ defaultPullSecret, isOcm = false, isPullSecretSet = false, - isSingleClusterFeatureEnabled = false, + isSingleClusterFeatureEnabled, }) => {Also applies to: 18-20, 33-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-lib/lib/common/components/clusters/PullSecret.tsx` around lines 11 - 12, The component PullSecret currently declares isSingleClusterFeatureEnabled as optional with a default false which causes silent fallback; change the prop in the component's props/interface to a required boolean (isSingleClusterFeatureEnabled: boolean) and remove any default assignment in the PullSecret function parameter list so callers must provide it; then update all call sites that render <PullSecret ...> to pass an explicit boolean value for isSingleClusterFeatureEnabled (do not rely on local fallback logic inside PullSecret), and run typechecks to ensure no remaining optional usages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libs/ui-lib/lib/common/components/clusters/PullSecret.tsx`:
- Around line 11-12: The component PullSecret currently declares
isSingleClusterFeatureEnabled as optional with a default false which causes
silent fallback; change the prop in the component's props/interface to a
required boolean (isSingleClusterFeatureEnabled: boolean) and remove any default
assignment in the PullSecret function parameter list so callers must provide it;
then update all call sites that render <PullSecret ...> to pass an explicit
boolean value for isSingleClusterFeatureEnabled (do not rely on local fallback
logic inside PullSecret), and run typechecks to ensure no remaining optional
usages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4938d02-33ed-48c9-81bd-9039dbacd488
📒 Files selected for processing (8)
apps/assisted-disconnected-ui/proxy/app.goapps/assisted-disconnected-ui/proxy/config/config.goapps/assisted-disconnected-ui/proxy/pullsecret/handler.golibs/locales/lib/en/translation.jsonlibs/ui-lib/lib/common/components/clusters/PullSecret.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/OcmClusterDetailsFormFields.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetails.tsxlibs/ui-lib/lib/ocm/hooks/usePullSecret.ts
✅ Files skipped from review due to trivial changes (1)
- apps/assisted-disconnected-ui/proxy/app.go
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetails.tsx
- libs/locales/lib/en/translation.json
- apps/assisted-disconnected-ui/proxy/config/config.go
- apps/assisted-disconnected-ui/proxy/pullsecret/handler.go
- libs/ui-lib/lib/ocm/hooks/usePullSecret.ts
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
ElayAharoni
force-pushed
the
pull-secret-auto-populated-in-ABI-local-UI
branch
from
1d06dbc to
32a8776
Compare
|
/cherrypick release-4.21 |
|
@ElayAharoni: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ElayAharoni, rawagner The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 787898c
into
openshift-assisted:master
|
@ElayAharoni: Jira Issue OCPBUGS-79666: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-79666 has been moved to the MODIFIED state. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@ElayAharoni: #3508 failed to apply on top of branch "release-4.21": DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
4 participants
https://redhat.atlassian.net/browse/OCPBUGS-79666
Summary by CodeRabbit
Release Notes