-
Notifications
You must be signed in to change notification settings - Fork 666
CONSOLE-4772: Update list pages to use DataView in Workloads Tab #15489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@krishagarwal278: This pull request references CONSOLE-4772 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 story to target the "4.21.0" version, but no target version was set. In 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. |
| </> | ||
| ); | ||
| const rowCells = { | ||
| name: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deduplicate the keys by pulling them out to a separate array of objects.
|
@krishagarwal278: This pull request references CONSOLE-4772 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 story to target the "4.21.0" version, but no target version was set. In 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. |
08f29af to
e90f74a
Compare
|
/retest |
afacb45 to
66c1116
Compare
66c1116 to
136ac60
Compare
|
@krishagarwal278: This pull request references CONSOLE-4772 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 story to target the "4.21.0" version, but no target version was set. In 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. |
|
/retest-required Seems flaky from the error log. |
| skipYamlReloadTest: true, | ||
| skipYamlSaveTest: true, | ||
| }); | ||
| // .set('buildconfigs', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still needed
| }); | ||
|
|
||
| // disabled as listPage.rows.shouldExist isn't a valid test | ||
| // disabled as listPage.dvRows.shouldExist isn't a valid test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code below didn't get updated, so this comment update is unnecessary.
| "Create {{label}}": "Create {{label}}", | ||
| "Edit {{label}}": "Edit {{label}}", | ||
| "{helpText}": "{helpText}", | ||
| "Disruptions allowed": "Disruptions allowed", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new string is indicative there are unintended changes somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import { useActiveColumns } from '@console/internal/components/factory/Table/active-columns-hook'; | ||
| import VirtualizedTable from '@console/internal/components/factory/Table/VirtualizedTable'; | ||
| import { getPDBTableColumns } from './pdb-table-columns'; | ||
| import PodDisruptionBudgetTableRow from './PDBTableRow'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the existing row is shared, we want to maintain that sharing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, i'll import and apply https://github.com/openshift/console/blob/main/frontend/packages/console-app/src/components/pdb/pdb-table-columns.tsx in the new table implementation. Won't be changing cell contents since it's out of scope 👍🏻
|
|
||
| const usePDBColumns = (): TableColumn<PodDisruptionBudgetKind>[] => { | ||
| const { t } = useTranslation(); | ||
| const [columns] = useActiveColumns({ columns: getPDBTableColumns() }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to keep column management.
| qs: { kind: 'Pod', q: 'app=name', name: WORKLOAD_NAME }, | ||
| }); | ||
| listPage.rows.shouldExist(WORKLOAD_NAME); | ||
| listPage.dvRows.shouldExist(WORKLOAD_NAME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is disabled, so no need to update.
| listPage.dvRows.shouldBeLoaded(); | ||
| cy.visit(`/k8s/ns/${testName}/cronjobs/${CRONJOB_NAME}/jobs`); | ||
| listPage.dvRows.countShouldBe(2); | ||
| listPage.rows.countShouldBe(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jobs list was already updated, so this change is gonna break the test.
| cell: <ResourceLink kind="Namespace" name={namespace} />, | ||
| }, | ||
| [tableColumnInfo[2].id]: { | ||
| cell: <span>{dataSize}</span>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed <span/> tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the unnecessary <span> is great! But it looks like the value of this cell changed from {_.size(configMap.data) + _.size(configMap.binaryData)} to sorts.dataSize(configMap).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the correct calculation for dataSize is in table.tsx:
https://github.com/openshift/console/blob/main/frontend/public/components/factory/table.tsx#L64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am importing sort from './factory/table' and applying sorts.dataSize to the cell
| cell: <ResourceLink kind="Namespace" name={namespace} />, | ||
| }, | ||
| [tableColumnInfo[2].id]: { | ||
| cell: <span>{cronjob.spec.schedule}</span>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contents are not the same as before. I'm gonna stop reviewing here and have you double check the rest of the files.
…ap, cronjob, PDBList, daemon-set)in workloads modified: frontend/packages/console-app/src/components/data-view/ResourceDataView.tsx modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-cli-download.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-notification.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-yaml-sample.cy.ts modified: frontend/public/components/configmap.tsx modified: frontend/public/components/deployment-config.tsx modified: frontend/public/components/deployment.tsx modified: frontend/public/components/factory/list-page.tsx modified: frontend/public/components/filter-toolbar.tsx modified: frontend/public/components/secret.tsx
modified: frontend/public/components/cron-job.tsx modified: frontend/public/components/secret.tsx modified: frontend/public/locales/en/public.json
modified: frontend/packages/integration-tests-cypress/tests/crud/namespace-crud.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crud/resource-crud.cy.ts modified: frontend/packages/integration-tests-cypress/views/list-page.ts modified: frontend/public/components/daemon-set.tsx modified: frontend/public/locales/en/public.json modified: frontend/packages/integration-tests-cypress/tests/crud/namespace-crud.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crud/resource-crud.cy.ts modified: frontend/packages/integration-tests-cypress/views/list-page.ts modified: frontend/public/components/daemon-set.tsx modified: frontend/public/locales/en/public.json
…orkload-table.tsx modified: frontend/packages/console-app/src/components/data-view/ResourceDataView.tsx modified: frontend/packages/console-app/src/components/pdb/PDBList.tsx modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-cli-download.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-external-log-link.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-link.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-notification.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-yaml-sample.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crud/namespace-crud.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crud/resource-crud.cy.ts modified: frontend/packages/integration-tests-cypress/views/list-page.ts modified: frontend/public/components/configmap.tsx modified: frontend/public/components/cron-job.tsx modified: frontend/public/components/daemon-set.tsx modified: frontend/public/components/deployment-config.tsx modified: frontend/public/components/deployment.tsx modified: frontend/public/components/secret.tsx modified: frontend/public/components/workload-table.tsx modified: frontend/public/locales/en/public.json modified: frontend/packages/console-app/src/components/pdb/PDBList.tsx modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-cli-download.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-external-log-link.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-link.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-notification.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-yaml-sample.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crud/namespace-crud.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crud/resource-crud.cy.ts modified: frontend/packages/integration-tests-cypress/views/list-page.ts modified: frontend/public/components/configmap.tsx modified: frontend/public/components/cron-job.tsx modified: frontend/public/components/daemon-set.tsx modified: frontend/public/components/deployment-config.tsx modified: frontend/public/components/deployment.tsx modified: frontend/public/components/secret.tsx modified: frontend/public/components/workload-table.tsx modified: frontend/public/locales/en/public.json
modified: frontend/public/components/modals/expand-pvc-modal.tsx
modified: frontend/packages/integration-tests-cypress/tests/app/demo-dynamic-plugin.cy.ts modified: frontend/packages/integration-tests-cypress/tests/app/deployments.cy.ts modified: frontend/packages/integration-tests-cypress/tests/app/filtering-and-searching.cy.ts modified: frontend/packages/integration-tests-cypress/tests/app/start-job-from-cronjob.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-cli-download.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-external-log-link.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-notification.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crd-extensions/console-yaml-sample.cy.ts modified: frontend/packages/integration-tests-cypress/tests/crud/resource-crud.cy.ts modified: frontend/packages/integration-tests-cypress/views/list-page.ts modified: frontend/public/components/configmap.tsx modified: frontend/public/components/daemon-set.tsx modified: frontend/public/components/workload-table.tsx modified: frontend/public/locales/en/public.json
97639ce to
083bb3d
Compare
|
/retest |
|
@krishagarwal278: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
Replaced by #15653 |
Updated CRD list pages(secret, deployment, deployment-config, daemon-set, PDB, configmap) in workloads
workloads.1.mp4
Note: I've only updated modals in this PR because of failing integration tests due to a conflict when pulling the useOverlay Hook story work