AGENT-1181: Refactor kubeconfig download to new design of credentials download#2905
Conversation
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@ElayAharoni: This pull request references AGENT-1181 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 sub-task to target the "4.19.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. |
WalkthroughThis update introduces several modifications to support a new credentials download process in the cluster installation flow. New localization entries have been added for error messages and user instructions. UI components now show a read-only username field using a text input and display loading states on wizard buttons. The cluster wizard flow has been updated to replace kubeconfig-download steps with a new credentials-download process, and a dedicated React component has been added to handle credentials fetching, download, and error notifications. Additionally, a new documentation link has been provided. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CDComponent as CredentialsDownload
participant API as ClustersAPI
participant Alert as AlertHandler
User->>CDComponent: Click "Download Credentials"
CDComponent->>API: getCredentials(cluster)
API-->>CDComponent: Return credentials data / error
alt Successful fetch
CDComponent->>User: Provide downloaded credentials
else Error occurred
CDComponent->>Alert: Trigger error alert
end
sequenceDiagram
participant WizardContext as ClusterWizardContextProvider
participant Wizard as ClusterWizard
participant Factory as ComponentFactory
WizardContext->>Wizard: Provide step identifiers (include "credentials-download")
Wizard->>Factory: Render component for "credentials-download"
Factory-->>Wizard: Return CredentialsDownload component
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
openshift-ci
bot
added
the
size/L
|
@ElayAharoni: This pull request references AGENT-1181 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 sub-task to target the "4.19.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. |
![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
🧹 Nitpick comments (5)
libs/ui-lib/lib/common/components/clusterDetail/ClusterCredentials.tsx (1)
108-111: Consider translating the "Username" stringThe "Username" label should be translated using the translation function for consistency with other text elements in this component, such as "Password" which uses
t('ai:Password').<DetailItem - title="Username" + title={t('ai:Username')} value={<TextInput value={credentials.username} readOnlyVariant="default" />} />libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (3)
124-131: Consider adding retry mechanism for credential fetchingIn case of credential fetching failures, it might be helpful to provide a retry button or mechanism to allow users to attempt to fetch credentials again without having to refresh the entire page.
You could add a simple retry button when an error occurs:
<Grid style={{ justifyItems: 'start' }}> <ClusterCredentials cluster={cluster} credentials={credentials} credentialsError={credentialsError} error={credentialsError !== ''} /> + {credentialsError && ( + <Button + variant="secondary" + onClick={fetchCredentials} + style={{ marginTop: '10px' }} + > + {t('ai:Retry fetching credentials')} + </Button> + )} </Grid>
52-66: Consider more descriptive error messagingThe current error handling provides basic feedback, but consider enhancing error messages to guide users on possible solutions.
handleApiError(e, (e) => { addAlert({ title: t(`Could not download ${fileName} file`), - message: getApiErrorMessage(e), + message: t('ai:${getApiErrorMessage(e)}. Please check your network connection and try again. If the problem persists, contact support.'), }); });
74-83: Consider adding more descriptive UI feedback during transitionsThe timeout used for the loading state is good, but users might benefit from a more explicit success message before navigation.
setTimeout(() => { setIsDownloading(false); if (!hasError) { + addAlert({ + title: t('ai:Credentials successfully downloaded'), + message: t('ai:You can now proceed to the next step'), + variant: AlertVariant.success, + }); clusterWizardContext.moveNext(); } }, 500);libs/locales/lib/en/translation.json (1)
415-415: User Prompt Grammar ImprovementThe new prompt key
"ai:I understand that I need to download credentials files prior of proceeding with the cluster installation."is added to inform users about the required action. For clarity and grammatical accuracy, consider changing"prior of proceeding"to"prior to proceeding".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/locales/lib/en/translation.json(4 hunks)libs/ui-lib/lib/common/components/clusterDetail/ClusterCredentials.tsx(2 hunks)libs/ui-lib/lib/common/components/ui/WizardFooter.tsx(3 hunks)libs/ui-lib/lib/common/config/docs_links.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizard.tsx(2 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (5)
libs/types/assisted-installer-service.d.ts (2)
Cluster(110-398)Credentials(756-760)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(25-31)libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)libs/ui-lib/lib/ocm/services/apis/index.ts (1)
ClustersAPI(5-5)libs/ui-lib/lib/common/config/docs_links.ts (1)
KUBECONFIG_INFO_LINK(186-187)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: tests
- GitHub Check: translation-files
- GitHub Check: circular-deps
- GitHub Check: lint
- GitHub Check: format
- GitHub Check: unit-tests
🔇 Additional comments (20)
libs/ui-lib/lib/common/config/docs_links.ts (1)
185-187: LGTM: New documentation link for Kubeconfig managementThe addition of the
KUBECONFIG_INFO_LINKconstant provides a useful documentation reference for users to learn about managing CLI profiles in OpenShift. This aligns well with the PR objective of refactoring the kubeconfig download process.libs/ui-lib/lib/common/components/clusterDetail/ClusterCredentials.tsx (1)
2-9: LGTM: Added TextInput component importThe import of the TextInput component from @patternfly/react-core is required for the UI enhancements in this file.
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx (1)
72-72: LGTM: Updated wizard step name to "credentials-download"The step name has been updated from 'kubeconfig-download' to 'credentials-download', which aligns with the new design and terminology focus on credentials rather than just kubeconfig.
libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts (1)
14-14: LGTM: Renamed step to "Download credentials"The wizard step name has been updated from "Download Kubeconfig" to "Download credentials", maintaining consistency with the PR's objective and other related file changes.
libs/ui-lib/lib/common/components/ui/WizardFooter.tsx (3)
29-29: LGTM: Good addition of loading state supportAdding the
isNextButtonLoadingproperty enhances the UI feedback during asynchronous operations.
55-55: LGTM: Proper prop destructuringThe new property is correctly destructured from the props.
73-73: LGTM: Well-implemented loading stateThe
isLoadingprop is correctly applied to the Button component to provide visual feedback during async operations.libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizard.tsx (2)
19-19: LGTM: Component import updatedImport statement properly updated to use the new CredentialsDownload component.
48-49: LGTM: Step handler updatedThe case statement is correctly updated to handle the 'credentials-download' step and render the new CredentialsDownload component with the required props.
libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts (4)
32-32: LGTM: Type definition updatedThe ClusterWizardStepsType is properly updated to use 'credentials-download' instead of 'kubeconfig-download'.
44-44: LGTM: Wizard steps order updatedThe wizardStepsOrder array correctly includes 'credentials-download' in place of 'kubeconfig-download'.
250-250: LGTM: Validation map renamedVariable naming correctly updated to reflect the new terminology.
265-265: LGTM: Validation map entry updatedThe wizardStepsValidationsMap correctly uses 'credentials-download' as the key for the validation map.
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (4)
22-46: LGTM: Well-structured component initialization and credentials fetchingThe component is well-organized with proper state management and a cleanly implemented credentials fetching function. Good error handling is in place to provide user feedback.
85-94: LGTM: Well-implemented footerThe ClusterWizardFooter correctly uses the new
isNextButtonLoadingprop to display loading state during download operations. The button is appropriately disabled based on the checkbox state and download progress.
100-114: LGTM: Informative alert messageGood implementation of an alert that provides important context to users about securely storing credentials, with a helpful link to learn more about Kubeconfig.
115-123: LGTM: User confirmation checkboxGood UX practice to require user acknowledgment before downloading sensitive credentials.
libs/locales/lib/en/translation.json (3)
330-330: Consistent Error Message EntryThe new translation key
"ai:Failed to load Credentials Download step"is added in a manner consistent with other error messages. The text is clear and directly communicates the failure state when the credentials download step cannot be loaded.
493-493: 'Learn more about Kubeconfig' Entry VerificationThe translation key
"ai:Learn more about Kubeconfig"is introduced with the same display text. This addition is consistent with the design changes and effectively supports the new credentials download functionality by offering users additional information.
517-517: Credentials Storage InstructionThe new translation key
"ai:Make sure you download and store your credentials files in a safe place"is clearly added to remind users to secure their credentials. The wording is consistent with similar instructional messages across the file.
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx
Outdated
Show resolved
Hide resolved
ElayAharoni
force-pushed
the
refactor-kubeconfig-download-to-new-design-of-credentials-download
branch
from
dd1695a to
574763f
Compare
|
@ElayAharoni: This pull request references AGENT-1181 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 sub-task to target the "4.19.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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (3)
68-83: Consider optimizing the download processThe current implementation downloads files sequentially and uses setTimeout for navigation. Consider these improvements:
- Download files in parallel using Promise.all for better performance
- Remove the setTimeout and navigate directly after handling the download results
const handleDownloadClick = async () => { setIsDownloading(true); - const configSuccess = await downloadSingleFile('kubeconfig'); - const passwordSuccess = await downloadSingleFile('kubeadmin-password'); + const [configSuccess, passwordSuccess] = await Promise.all([ + downloadSingleFile('kubeconfig'), + downloadSingleFile('kubeadmin-password') + ]); const hasError = !configSuccess || !passwordSuccess; setIsDownloading(false); - setTimeout(() => { - setIsDownloading(false); - - if (!hasError) { - clusterWizardContext.moveNext(); - } - }, 500); + if (!hasError) { + clusterWizardContext.moveNext(); + } };
116-123: Consider using functional updates for checkbox stateFor better practice when updating state based on previous state, consider using the functional form of setState.
- onChange={() => setIsChecked(!isChecked)} + onChange={() => setIsChecked(prev => !prev)}
22-29: Consider adding loading state for initial credentials fetchThe component doesn't show a loading state while initially fetching credentials. Consider adding a loading state to improve user experience during the initial data fetch.
const CredentialsDownload: React.FC<{ cluster: Cluster }> = ({ cluster }) => { const [isChecked, setIsChecked] = React.useState<boolean>(false); const clusterWizardContext = useClusterWizardContext(); const [credentials, setCredentials] = React.useState<Credentials>(); const [credentialsError, setCredentialsError] = React.useState(''); const [isDownloading, setIsDownloading] = React.useState(false); + const [isLoading, setIsLoading] = React.useState(false); const { addAlert } = useAlerts(); const { t } = useTranslation();Then update the fetchCredentials function:
const fetchCredentials = React.useCallback(() => { const fetch = async () => { setCredentialsError(''); + setIsLoading(true); if (!cluster.id) { + setIsLoading(false); return; } try { const response = await ClustersAPI.getCredentials(cluster.id); const data = { username: response.data.username, password: response.data.password }; setCredentials(data); } catch (err) { setCredentialsError(`Failed to fetch credentials, ${getErrorMessage(err)}`); } finally { + setIsLoading(false); } }; void fetch(); }, [cluster]);And pass this to the ClusterCredentials component:
<ClusterCredentials cluster={cluster} credentials={credentials} credentialsError={credentialsError} error={credentialsError !== ''} + isLoading={isLoading} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/locales/lib/en/translation.json(4 hunks)libs/ui-lib/lib/common/components/clusterDetail/ClusterCredentials.tsx(2 hunks)libs/ui-lib/lib/common/components/ui/WizardFooter.tsx(3 hunks)libs/ui-lib/lib/common/config/docs_links.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizard.tsx(2 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- libs/ui-lib/lib/common/config/docs_links.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx
- libs/ui-lib/lib/common/components/clusterDetail/ClusterCredentials.tsx
- libs/ui-lib/lib/common/components/ui/WizardFooter.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizard.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts
- libs/locales/lib/en/translation.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (5)
libs/types/assisted-installer-service.d.ts (2)
Cluster(110-398)Credentials(756-760)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(25-31)libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)libs/ui-lib/lib/ocm/services/apis/index.ts (1)
ClustersAPI(5-5)libs/ui-lib/lib/common/config/docs_links.ts (1)
KUBECONFIG_INFO_LINK(186-187)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests
- GitHub Check: unit-tests
- GitHub Check: format
- GitHub Check: lint
🔇 Additional comments (6)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (6)
31-46: Well implemented credential fetchingThe fetchCredentials function is well structured with proper error handling and state management. Good use of useCallback with the correct dependency array to optimize performance.
48-50: Clean effect hook implementationThe useEffect hook correctly calls fetchCredentials when the component mounts and when the cluster dependency changes.
52-66: Good error handling in downloadSingleFileThe downloadSingleFile function properly handles API errors and provides appropriate user feedback through alerts.
85-94: Good implementation of the wizard footerThe footer component correctly handles navigation, button states, and loading indicators.
100-114: Good user guidance with alert and documentation linkThe alert effectively communicates the importance of securely storing credentials and provides a helpful link to additional documentation.
124-131: Clean implementation of credentials displayGood use of the ClusterCredentials component to display the fetched credentials with proper error state handling.
libs/ui-lib/lib/common/components/clusterDetail/ClusterCredentials.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx
Outdated
Show resolved
Hide resolved
ElayAharoni
force-pushed
the
refactor-kubeconfig-download-to-new-design-of-credentials-download
branch
from
f0edb15 to
e3b62ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (2)
98-98: Localize the “Download credentials” title.
This heading is not wrapped with youruseTranslationhook. Wrap the string int('')for full i18n coverage.
114-114: Avoid Custom Styling in favor of PatternFly layouts.
This duplicates previous feedback about using PatternFly's layout components (e.g.,<Stack hasGutter>or built-in spacing tokens). Inline margin styles can often be replaced with the recommended PF approach.
🧹 Nitpick comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (2)
24-27: Consider using explicit default values for your React states.
Defining default values (e.g.,React.useState<Credentials | undefined>(undefined)) can provide clearer intent and reduce potential undefined checks. This is a minor improvement for clarity.
66-82: Revisit the 500ms timeout.
Using asetTimeoutto resetisDownloadingmight lead to UI inconsistencies in edge cases or overly delay the user's next action if the download completes quickly. Consider removing this artificial delay or using a more adaptive approach (e.g., reset state immediately after downloads complete).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/locales/lib/en/translation.json(6 hunks)libs/ui-lib/lib/common/components/clusterDetail/ClusterCredentials.tsx(2 hunks)libs/ui-lib/lib/common/components/ui/WizardFooter.tsx(3 hunks)libs/ui-lib/lib/common/config/docs_links.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizard.tsx(2 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- libs/ui-lib/lib/common/config/docs_links.ts
- libs/ui-lib/lib/common/components/clusterDetail/ClusterCredentials.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizard.tsx
- libs/ui-lib/lib/common/components/ui/WizardFooter.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts
- libs/locales/lib/en/translation.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (7)
libs/types/assisted-installer-service.d.ts (2)
Cluster(110-398)Credentials(756-760)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(25-31)libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)libs/ui-lib/lib/ocm/services/apis/index.ts (1)
ClustersAPI(5-5)libs/ui-lib/lib/common/api/utils.ts (2)
handleApiError(19-51)getApiErrorMessage(53-58)libs/ui-lib/lib/common/utils.ts (2)
getErrorMessage(13-21)downloadFile(92-108)libs/ui-lib/lib/common/config/docs_links.ts (1)
KUBECONFIG_INFO_LINK(186-187)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: tests
- GitHub Check: translation-files
- GitHub Check: circular-deps
- GitHub Check: format
- GitHub Check: unit-tests
- GitHub Check: lint
🔇 Additional comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (1)
50-64: Looks good!
ThedownloadSingleFilemethod correctly uses async/await to fetch credential files and handles errors withhandleApiError. Overall, this section appears solid.
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx
Outdated
Show resolved
Hide resolved
ElayAharoni
force-pushed
the
refactor-kubeconfig-download-to-new-design-of-credentials-download
branch
from
e3b62ec to
066bccb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (2)
114-115: Avoid inline stylingAs mentioned in previous review comments, custom inline styling should be avoided when possible. Use PatternFly spacing utilities or components that provide margin/padding options.
- <Checkbox - style={{ marginBottom: '50px' }} - id="credentials-download-agreement" + <Checkbox + className="pf-v5-u-mb-xl" + id="credentials-download-agreement"
122-122: Replace inline styling with PatternFly utilitiesInline styling should be replaced with PatternFly layout components or utility classes.
- <Grid style={{ justifyItems: 'start' }}> + <Grid hasGutter className="pf-v5-u-justify-content-flex-start">
🧹 Nitpick comments (3)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (3)
58-59: Internationalize error message title correctlyThe error message title should use the i18n framework with the 'ai:' namespace prefix, similar to other translated strings in the file.
- title: t(`Could not download ${fileName} file`), + title: t('ai:Could not download {{fileName}} file', { fileName }),
98-98: Missing internationalization for header textThe header text "Download credentials" should be internationalized using the translation function with the 'ai:' namespace prefix, similar to other translated strings in the component.
- <ClusterWizardStepHeader>Download credentials</ClusterWizardStepHeader> + <ClusterWizardStepHeader>{t('ai:Download credentials')}</ClusterWizardStepHeader>
66-81: Consider adding success feedbackThe
handleDownloadClickfunction handles downloads and error cases, but doesn't provide positive feedback when downloads succeed. Consider adding a success alert to improve user experience.const handleDownloadClick = async () => { setIsDownloading(true); const configSuccess = await downloadSingleFile('kubeconfig'); const passwordSuccess = await downloadSingleFile('kubeadmin-password'); const hasError = !configSuccess || !passwordSuccess; setTimeout(() => { setIsDownloading(false); if (!hasError) { + addAlert({ + title: t('ai:Credentials downloaded successfully'), + variant: AlertVariant.success, + }); clusterWizardContext.moveNext(); } }, 500); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/locales/lib/en/translation.json(6 hunks)libs/ui-lib/lib/common/components/clusterDetail/ClusterCredentials.tsx(2 hunks)libs/ui-lib/lib/common/components/ui/WizardFooter.tsx(3 hunks)libs/ui-lib/lib/common/config/docs_links.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizard.tsx(2 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- libs/ui-lib/lib/common/config/docs_links.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx
- libs/ui-lib/lib/common/components/clusterDetail/ClusterCredentials.tsx
- libs/ui-lib/lib/common/components/ui/WizardFooter.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizard.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts
- libs/locales/lib/en/translation.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (7)
libs/types/assisted-installer-service.d.ts (2)
Cluster(110-398)Credentials(756-760)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(25-31)libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)libs/ui-lib/lib/ocm/services/apis/index.ts (1)
ClustersAPI(5-5)libs/ui-lib/lib/common/api/utils.ts (1)
getApiErrorMessage(53-58)libs/ui-lib/lib/common/utils.ts (2)
getErrorMessage(13-21)downloadFile(92-108)libs/ui-lib/lib/common/config/docs_links.ts (1)
KUBECONFIG_INFO_LINK(186-187)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: circular-deps
- GitHub Check: tests
- GitHub Check: lint
- GitHub Check: format
🔇 Additional comments (5)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (5)
31-48: Correct implementation of data fetchingGood job implementing the feedback to combine useCallback and useEffect. The current implementation correctly fetches credentials when the component mounts or when the cluster ID changes.
69-70: Fixed filename issueThe incorrect kubeconfig filename has been fixed as suggested in the previous review.
97-97: Good use of PatternFly layoutWell done using the
Stack hasGuttercomponent as suggested in the previous review instead of custom styling.
127-127: Fixed error condition checkThe error condition has been correctly updated to use a proper truthy check with
!!credentialsErroras suggested in the previous review.
50-64:Details
✅ Verification successful
Check for consistent error handling approach
The
downloadSingleFilefunction useshandleApiErrorcorrectly, but you should ensure the error messages follow the same pattern across the application. Consider extracting common error handling to a utility function if this pattern is repeated elsewhere.
🏁 Script executed:
#!/bin/bash # Check if similar error handling patterns exist in other files rg -A 3 "handleApiError" --glob "*.tsx" | grep -v "CredentialsDownload.tsx"Length of output: 25539
Consistent Error Handling Verified
The
downloadSingleFilefunction’s error handling usinghandleApiErroraligns with the patterns observed across the codebase. Searches in similar modules (e.g., inResetSingleClusterModal.tsx,ClusterDetails.tsx, and others) confirm that the approach—invokinghandleApiErrorwith an inline callback that callsaddAlertusing a title andgetApiErrorMessage(e)—is applied consistently.
- The same error handling pattern is reused in multiple files.
- Consider extracting this common logic to a utility function if you predict further repetition, but note that the error messages are tailored per context, so any refactoring should preserve the necessary specificity.
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
ElayAharoni
force-pushed
the
refactor-kubeconfig-download-to-new-design-of-credentials-download
branch
from
066bccb to
18b9681
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (1)
126-126: Correct error condition check implementation.You've successfully implemented the suggested fix to use
!!credentialsErrorinstead ofcredentialsError !== ''for the error condition check. This correctly handles both undefined and empty string cases.
🧹 Nitpick comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (2)
66-81: Consider extracting the timeout duration to a named constant.The 500ms timeout in the
handleDownloadClickfunction works well for providing UI feedback, but hardcoded values reduce code maintainability. Consider extracting this to a named constant at the top of the file or component.+ const DOWNLOAD_FEEDBACK_DELAY_MS = 500; const handleDownloadClick = async () => { // ... setTimeout(() => { setIsDownloading(false); if (!hasError) { clusterWizardContext.moveNext(); } - }, 500); + }, DOWNLOAD_FEEDBACK_DELAY_MS); };
94-132: Consider replacing inline style with PatternFly components.Great job using
<Stack hasGutter>as suggested in previous comments. However, there's still an inline style on the Grid component. Consider using PatternFly layout components or modifiers instead of custom styling when possible.- <Grid hasGutter style={{ justifyItems: 'start' }}> + <Grid hasGutter className="pf-m-align-left">or if you want the grid items to start-align:
- <Grid hasGutter style={{ justifyItems: 'start' }}> + <Grid hasGutter alignItems="flex-start">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/locales/lib/en/translation.json(7 hunks)libs/ui-lib/lib/common/components/clusterDetail/ClusterCredentials.tsx(2 hunks)libs/ui-lib/lib/common/components/ui/WizardFooter.tsx(3 hunks)libs/ui-lib/lib/common/config/docs_links.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizard.tsx(2 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- libs/ui-lib/lib/common/components/clusterDetail/ClusterCredentials.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizard.tsx
- libs/ui-lib/lib/common/config/docs_links.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts
- libs/ui-lib/lib/common/components/ui/WizardFooter.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts
- libs/locales/lib/en/translation.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (7)
libs/types/assisted-installer-service.d.ts (2)
Cluster(110-398)Credentials(756-760)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(25-31)libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)libs/ui-lib/lib/ocm/services/apis/index.ts (1)
ClustersAPI(5-5)libs/ui-lib/lib/common/api/utils.ts (1)
getApiErrorMessage(53-58)libs/ui-lib/lib/common/utils.ts (2)
getErrorMessage(13-21)downloadFile(92-108)libs/ui-lib/lib/common/config/docs_links.ts (1)
KUBECONFIG_INFO_LINK(186-187)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests
- GitHub Check: translation-files
- GitHub Check: format
- GitHub Check: lint
🔇 Additional comments (4)
libs/ui-lib/lib/ocm/components/clusterWizard/CredentialsDownload.tsx (4)
50-64: Well-structured downloadSingleFile function with proper error handling.The function properly handles downloading credential files, shows appropriate error messages using the alert system, and returns a success/failure status that's used by the parent function. Good use of the
handleApiErrorutility as requested in previous comments.
31-48: Clean implementation of credentials fetching.Good job incorporating the previous feedback to consolidate the logic into a single
useEffecthook. The credentials fetching logic properly resets errors, handles the API call, and manages state updates in a clean way.
22-29: State management looks good with proper types.The component's state is well-organized with appropriate types. Nice job implementing the previous feedback to use
undefinedas the default forcredentialsErrorinstead of an empty string.
83-92: Well-implemented footer with appropriate button states.The footer component correctly manages the disabled state of the download button based on checkbox status and download progress. The loading state is also properly handled during downloads.
…-of-credentials-download
openshift-ci
bot
added
lgtm
|
/approve |
|
[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 b47bbab
into
openshift-assisted:master
… download (openshift-assisted#2905) * refactor kubeconfig download to credentials download Signed-off-by: Elay Aharoni <elayaha@gmail.com> * Download credentials page Signed-off-by: Elay Aharoni <elayaha@gmail.com> --------- Signed-off-by: Elay Aharoni <elayaha@gmail.com>
3 participants
https://issues.redhat.com/browse/AGENT-1181
refactored the kubeconfig download page to the new designed credentials download.
added functionality to show the credentials and some alerts and error when necessary.
also added download for both kubeadmin-password and kubeconfig file.
added video of the behavior when there is an error, and when there are no.
https://www.loom.com/share/be95459a53a144a0b7fe49c2c8ae7799?sid=d0431c59-63ae-4bed-89a5-b0c70662e1e6
Summary by CodeRabbit
New Features
Documentation