MGMT-24493: Allow HTTPS proxy for Discovery ISO#3777
Conversation
|
@LiorSoffer: This pull request references MGMT-24493 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. 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
added
the
jira/valid-reference
openshift-ci
Bot
added
approved
LiorSoffer
added
do-not-merge
📝 WalkthroughWalkthroughAdds dedicated HTTP/HTTPS proxy validation across multiple forms, updates localized validation messages and translation guidance, and extracts ChangesHTTPS Proxy Configuration Support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NetworkForm
participant httpsProxyValidationSchema
participant URLParser
participant i18next
NetworkForm->>httpsProxyValidationSchema: validate httpsProxy
httpsProxyValidationSchema->>URLParser: parse HTTP or HTTPS URL
httpsProxyValidationSchema->>i18next: resolve validation message
i18next-->>NetworkForm: localized validation result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
openshift-ci
Bot
added
the
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.cursor/rules/translations.mdc:
- Around line 45-48: The Trans i18nKey should be the stable translation key
string, not a localized value returned by t(), so update ProxyFields.tsx to stop
passing i18nKey={supportHttps ? t('ai:...') : t('ai:...')} and instead use
i18nKey={supportHttps ? 'ai:...' : 'ai:...'} (or compute the key string via a
small helper) and render t(...) only where you need the translated text; also
adjust the guidance in .cursor/rules/translations.mdc to reflect that Trans
expects key strings like "ai:..." and that using t() inside i18nKey will break
lookups at runtime even though parser extraction may succeed.
🪄 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: 34ce6150-9525-4c0c-b283-eee69a7cfc0e
📒 Files selected for processing (6)
.cursor/rules/translations.mdclibs/locales/lib/en/translation.jsonlibs/ui-lib/lib/common/components/clusterConfiguration/DiscoveryImageConfigForm.tsxlibs/ui-lib/lib/common/components/clusterConfiguration/ProxyFields.tsxlibs/ui-lib/lib/common/validationSchemas/proxyValidation.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsx
openshift-ci
Bot
added
the
approved
openshift-ci
Bot
added
size/XS
Signed-off-by: Lior Soffer <liorsoffer1@gmail.com>
Signed-off-by: Lior Soffer <liorsoffer1@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LiorSoffer 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LiorSoffer 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-ci
Bot
added
approved
There was a problem hiding this comment.
🧹 Nitpick comments (4)
libs/ui-lib/lib/common/components/clusterConfiguration/ProxyFields.tsx (1)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap translation props with
t().As per coding guidelines, always wrap strings used in
i18nKeyor other translation props witht('ai:…'); do not use bare string literals.However, since the
Transcomponent fromreact-i18nexttypically relies on a static string key for extraction and rendering, please verify if dynamically resolving it witht()causes any issues.♻️ Proposed refactor
- i18nKey={'ai:URL must start with <bold>http</bold> or <bold>https</bold>.'} + i18nKey={t('ai:URL must start with <bold>http</bold> or <bold>https</bold>.')}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/ui-lib/lib/common/components/clusterConfiguration/ProxyFields.tsx` at line 67, Update the translation prop in ProxyFields to pass the result of the i18n translation function t() using the existing ai-prefixed key instead of a bare string literal, while preserving the Trans component’s expected rendering and extraction behavior.Source: Coding guidelines
libs/ui-lib/lib/cim/components/modals/DiscoveryImageConfigForm.tsx (1)
28-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the barrel for the exported clusterConfiguration components.
UploadSSH,ProxyFields, andStaticIPInfocan come fromlibs/ui-lib/lib/common/components/clusterConfiguration;DiscoveryImageTypeControlGroupstill needs its direct import unless it’s added to the barrel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/ui-lib/lib/cim/components/modals/DiscoveryImageConfigForm.tsx` around lines 28 - 31, Update the imports in DiscoveryImageConfigForm to load UploadSSH, ProxyFields, and StaticIPInfo from the clusterConfiguration barrel export. Keep DiscoveryImageTypeControlGroup’s direct import unchanged unless it is explicitly added to that barrel.Source: Coding guidelines
libs/ui-lib/lib/cim/components/InfraEnv/InfraEnvFormPage.tsx (1)
89-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove stale comment.
With the switch to the dedicated
httpsProxyValidationSchema, the comment stating that the schema is shared because HTTPS is not supported is now outdated.♻️ Proposed refactor
httpsProxy: httpsProxyValidationSchema({ values, pairValueName: 'httpProxy', allowEmpty: true, t, - }), // share the schema, httpS is currently not supported + }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/ui-lib/lib/cim/components/InfraEnv/InfraEnvFormPage.tsx` around lines 89 - 94, Remove the stale inline comment after the httpsProxyValidationSchema configuration in InfraEnvFormPage, leaving the dedicated HTTPS validation schema usage unchanged.libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/NetworkStep/NetworkStep.tsx (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove stale comment.
Since
httpsProxynow uses its own dedicated validation schema and supports HTTPS URLs, the comment indicating that the schema is shared because "httpS is currently not supported" is outdated and should be removed.♻️ Proposed refactor
- httpsProxy: httpsProxyValidationSchema({ values, pairValueName: 'httpProxy', t }), // share the schema, httpS is currently not supported + httpsProxy: httpsProxyValidationSchema({ values, pairValueName: 'httpProxy', t }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/NetworkStep/NetworkStep.tsx` at line 33, Remove the stale inline comment from the httpsProxy validation schema configuration in NetworkStep, leaving the dedicated httpsProxyValidationSchema call and its arguments unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/NetworkStep/NetworkStep.tsx`:
- Line 33: Remove the stale inline comment from the httpsProxy validation schema
configuration in NetworkStep, leaving the dedicated httpsProxyValidationSchema
call and its arguments unchanged.
In `@libs/ui-lib/lib/cim/components/InfraEnv/InfraEnvFormPage.tsx`:
- Around line 89-94: Remove the stale inline comment after the
httpsProxyValidationSchema configuration in InfraEnvFormPage, leaving the
dedicated HTTPS validation schema usage unchanged.
In `@libs/ui-lib/lib/cim/components/modals/DiscoveryImageConfigForm.tsx`:
- Around line 28-31: Update the imports in DiscoveryImageConfigForm to load
UploadSSH, ProxyFields, and StaticIPInfo from the clusterConfiguration barrel
export. Keep DiscoveryImageTypeControlGroup’s direct import unchanged unless it
is explicitly added to that barrel.
In `@libs/ui-lib/lib/common/components/clusterConfiguration/ProxyFields.tsx`:
- Line 67: Update the translation prop in ProxyFields to pass the result of the
i18n translation function t() using the existing ai-prefixed key instead of a
bare string literal, while preserving the Trans component’s expected rendering
and extraction behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c763e394-a728-4681-ab86-1fe42fb812b1
📒 Files selected for processing (15)
.cursor/rules/translations.mdclibs/locales/lib/en/translation.jsonlibs/ui-lib/lib/cim/components/ClusterDeployment/use-networking-formik.tslibs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/NetworkStep/NetworkStep.tsxlibs/ui-lib/lib/cim/components/InfraEnv/InfraEnvFormPage.tsxlibs/ui-lib/lib/cim/components/modals/AddHostModal.tsxlibs/ui-lib/lib/cim/components/modals/DiscoveryImageConfigForm.tsxlibs/ui-lib/lib/cim/components/modals/EditProxyModal.tsxlibs/ui-lib/lib/cim/components/modals/types.tslibs/ui-lib/lib/common/components/clusterConfiguration/DownloadIso.tsxlibs/ui-lib/lib/common/components/clusterConfiguration/ProxyFields.tsxlibs/ui-lib/lib/common/components/clusterConfiguration/StaticIPInfo.tsxlibs/ui-lib/lib/common/components/clusterConfiguration/index.tslibs/ui-lib/lib/common/validationSchemas/proxyValidation.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- .cursor/rules/translations.mdc
|
/retest |
2 participants
https://redhat.atlassian.net/browse/MGMT-24493
BE: openshift/assisted-service#10589
Summary by CodeRabbit
New Features
Bug Fixes