Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for file uploads in S3 browser #1635

Merged
merged 1 commit into from
Oct 18, 2024
Merged

Add support for file uploads in S3 browser #1635

merged 1 commit into from
Oct 18, 2024

Conversation

bipuladh
Copy link
Contributor

screen-capture.webm

SanjalKatiyar
SanjalKatiyar reviewed Oct 10, 2024
View reviewed changes
packages/odf/components/s3-browser/bucket-overview/BucketOverview.tsx
Comment on lines -190 to -197
{fresh ? (
<HorizontalNav
pages={navPages}
resource={isCreatedByOBC && noobaaObjectBucket}
/>
) : (
<LoadingBox />
)}
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have multiple tabs under this HorizontalNav (Details, Objects, YAML, more in future)... idea was to mount/un-mount entire HorizontalNav on "refresh" rather than handling it for each tab again and again.

It's fine IMO if refreshing is removing the sidebar and cancelling uploading (we can "abortAll" on unmount), that's the intended affect of manual refreshing. User is intentionally clicking on that button.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are refreshing because the list of object is not being watched. However the details page and YAML wrapper should be using watchResource so there is no need to refresh the whole page. The user might want to refresh the table only to see the files being shown when the upload is going on especially in cases of large files being uploaded.

Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are only using watchResource at a single place, i.e., for reading OB CRs... other than that all are REST HTTPs calls...
useSWR might revalidate data from time to time, but currently we were not using any specific continuous/interval polling (because designs had "Refresh" option)... so clicking "Refresh" should mount/un-mount entire HorizontalNav (which includes Details tab as well).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might consider using continuous polling for Details tab, but as per designs this "Refresh" is on top of the page which refreshes everything, not just objects list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all other details page we are using watchResource so I am not sure why we would want to change the pattern here and ask users to press the refresh button for updating the details page. The other page in question is the YAML editor which should be constantly watching as well. So I am not sure about the need to refresh all tabs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because other places we read data directly from CRs only, here we need NooBaa endpoint which is REST HTTPs server. Only OB CRs are being watched (in the parent component, which is not affected by "refresh"), rest all info in Details tab and future S3 based tabs as well will not be CR based.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for YAML as well, editor will mount/un-mount but OB CR is passed as a prop from a upper component which is not affected by the refresh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default behaviour of details page should be to poll. We shouldn't have to refresh details page to get more info. If we need to introduce polling let's do that but details page being updated after refresh is an-anti pattern in our UI

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO that's not a good idea... because with that reasoning we should then poll in object list page too, we don't need "Refresh" at all...
let's discuss this offline once and try to sort this...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the code with the following updates:

  1. Details page get refreshed
  2. YAML page is a huge component so not refreshing it

@SanjalKatiyar
Copy link
Collaborator

packages/odf/components/s3-browser/upload-objects/index.ts is an empty file...

SanjalKatiyar
SanjalKatiyar reviewed Oct 11, 2024
View reviewed changes
packages/odf/components/s3-browser/upload-objects/FileUploadComponent.tsx Outdated
</FlexItem>
<FlexItem>
<Title headingLevel="h5">
{t('Drag and drop files/folders here.')}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{t('Drag and drop files/folders here.')}
{t('Drag and drop files/folders here or upload.')}

SanjalKatiyar
SanjalKatiyar reviewed Oct 17, 2024
View reviewed changes
packages/odf/components/s3-browser/bucket-overview/BucketOverview.tsx Outdated Show resolved Hide resolved
packages/shared/src/s3/commands.ts Show resolved Hide resolved
packages/shared/src/s3/commands.ts Show resolved Hide resolved
packages/odf/components/s3-browser/upload-objects/upload-component/uploads.ts Outdated Show resolved Hide resolved
packages/odf/components/s3-browser/upload-objects/upload-component/FileUploadComponent.tsx
Comment on lines +101 to +102
{(uploadStatus === UploadStatus.UPLOAD_START ||
uploadStatus === UploadStatus.UPLOAD_COMPLETE) && (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what will we do if all objects failed ?? won't we show any alert ??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would show alerts but we would still reach UPLOAD_COMPELTE state.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack !

packages/odf/components/s3-browser/upload-objects/upload-component/uploads.ts Outdated
Comment on lines 56 to 51
try {
await Promise.allSettled(allUploadPromise);
} catch (e) {
// eslint-disable-next-line no-console
console.error(e);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we set status as UploadStatus.UPLOAD_FAILED for the files which were rejected or when entire try block encounters any exception ??

packages/odf/components/s3-browser/upload-objects/upload-component/FileUploadComponent.tsx Outdated Show resolved Hide resolved
alfonsomthd
alfonsomthd reviewed Oct 17, 2024
View reviewed changes
packages/odf/components/s3-browser/upload-objects/UploadSidebar.tsx Outdated
Comment on lines 132 to 134
objects, something or the other will happen. We need to improve this
alert. Or not show this. Doesnt make sense to repeat your messages
in UI.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know microcopy is pending, still good not to show wrong text on UI !

Suggested change
objects, something or the other will happen. We need to improve this
alert. Or not show this. Doesnt make sense to repeat your messages
in UI.
objects,

@alfonsomthd
Copy link
Collaborator

@bipuladh rebase needed as Bucket Details Tab PR has been recently merged.

@bipuladh bipuladh force-pushed the file-upload branch 3 times, most recently from b83a673 to 1c9b135 Compare October 18, 2024 04:37
SanjalKatiyar
SanjalKatiyar reviewed Oct 18, 2024
View reviewed changes
packages/odf/components/s3-browser/bucket-details/BucketDetails.tsx Outdated
const { t } = useCustomTranslation();

return (
return refresh && resource ? (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return refresh && resource ? (
return refresh ? (

packages/odf/components/s3-browser/bucket-details/BucketDetails.tsx Outdated Show resolved Hide resolved
packages/odf/components/s3-browser/upload-objects/UploadSidebar.tsx Outdated
Comment on lines 132 to 134
objects, something or the other will happen. We need to improve this
alert. Or not show this. Doesnt make sense to repeat your messages
in UI.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know microcopy is pending, still good not to show wrong text on UI !

Suggested change
objects, something or the other will happen. We need to improve this
alert. Or not show this. Doesnt make sense to repeat your messages
in UI.
objects,

plugins/odf/console-extensions.json Outdated Show resolved Hide resolved
SanjalKatiyar
SanjalKatiyar reviewed Oct 18, 2024
View reviewed changes
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bipuladh
Add support for S3 file uploads
a5aeba0 
Signed-off-by: Bipul Adhikari <[email protected]>
@alfonsomthd
Copy link
Collaborator

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 18, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alfonsomthd, bipuladh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [alfonsomthd,bipuladh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit c5952c7 into red-hat-storage:master Oct 18, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SanjalKatiyar SanjalKatiyar SanjalKatiyar left review comments

@alfonsomthd alfonsomthd Awaiting requested review from alfonsomthd

Assignees

@alfonsomthd alfonsomthd

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bipuladh @SanjalKatiyar @alfonsomthd @openshift-merge-robot