import UI implementation #3352

eden-ohana · 2022-05-11T15:48:44Z

johnnyaug

Exciting feature!

Not done going over it yet, but going away for today so dropping my comments.

webui/src/lib/api/index.js

johnnyaug · 2022-05-11T21:37:57Z

webui/src/lib/components/repository/tree.jsx

+
+            <Row className="pt-2 ml-2">
+                <DotIcon className="mr-1 mt-1"/>
+                Or, click to&nbsp;<Button variant="link" onClick={onImport}>Import</Button>&nbsp;from the object store.


This should be the first one.

Also - regarding the "see the docs" one - it doesn't really make sense any more because it leads to the import docs page - so either rephrase the text it to make sense ("learn more bla bla") OR link to another page OR remove it.

Done, combined then together. I think it is imported to reference other ways to import as well.

webui/src/pages/repositories/repository/objects.jsx

johnnyaug · 2022-05-12T16:12:38Z

Still missing the "Test" button to check the connectivity to the import source.
Is that planned for later?

webui/src/pages/repositories/repository/objects.jsx

eden-ohana · 2022-05-13T11:28:40Z

@johnnyaug Thanks for the great UX comments!
For the test connection button, we need to implement server functionality first, AFAIK it is not a high priority for now.

johnnyaug

Sorry for the gradual review.

webui/package.json

webui/src/lib/api/index.js

webui/src/pages/repositories/repository/objects.jsx

johnnyaug · 2022-05-13T16:46:32Z

webui/src/pages/repositories/repository/objects.jsx

+
+            try {
+                importBranchResp = await branches.get(repo.id, importBranch)
+            } catch (error){


Suggested change

} catch (error){

} catch (error){

I think we should verify that the error here is that the branch wasn't found.

Right, the import can also fail if there are uncommitted changes for example.

johnnyaug · 2022-05-13T16:47:59Z

webui/src/pages/repositories/repository/objects.jsx

+
+    if (!reference || reference.type !== RefTypeBranch) return <></>
+
+    const checkStorageNamespaceValidity = () => {


All the mentions "storage namespace" should be changed to something more meaningful.

johnnyaug · 2022-05-13T16:51:24Z

webui/src/pages/repositories/repository/objects.jsx

+            const importBranch = `_${reference.id}_imported`;
+            const commitMsg = commitMsgRef.current.value;
+            const sourceRefVal = sourceRef.current.value;
+            const range_arr = []


Suggested change

const range_arr = []

const rangeArr = []

johnnyaug · 2022-05-13T16:53:21Z

webui/src/pages/repositories/repository/objects.jsx

+                    </Button>
+                    <Button variant="success" disabled={importState.inProgress || !importState.isStorageNamespaceValid} onClick={() => {
+                        if (importState.inProgress) return;
+                        ingest()


Is this called ingest because import is a reserved keyword?
You can use "doImport"

johnnyaug · 2022-05-13T17:02:14Z

webui/src/pages/repositories/repository/objects.jsx


-const UploadButton = ({ config, repo, reference, path, onDone, variant = "success", onClick, onHide, show = false}) => {
+const ImportButton = ({ config, repo, reference, path, onDone, onClick, variant = "success", onHide, show = false, enabled= false}) => {


Suggested change

const ImportButton = ({ config, repo, reference, path, onDone, onClick, variant = "success", onHide, show = false, enabled= false}) => {

const ImportButton = ({ config, repo, reference, path, onDone, onClick, variant = "success", onHide, show = false, enabled = false}) => {

Please format everything (only lines you changed, if possible).

itaiad200

Played with it, looks amazing! Added some comments.

itaiad200 · 2022-05-14T19:57:28Z

webui/src/pages/repositories/repository/objects.jsx

+            let prepend = `${destRef.current.value}`;
+            let importBranchResp
+            let sum = importState.numObj
+            const importBranch = `_${reference.id}_imported`;


The might lead to a confusing behaviour. If I'm on the imported branch, let's say _main_imported, I wouldn't expect to create another branch named __main_imported_imported. Maybe check for the _ prefix and _imported prefix, and just use the same branch if they match?

itaiad200 · 2022-05-14T19:59:24Z

webui/src/lib/api/index.js

+        const requestURL = QueryString.stringifyUrl({url: `/repositories/${repoId}/branches/${branchId}/commits`, query: {source_metarange: source_metarange}});
+        const parsedURL = QueryString.exclude(requestURL, (name, value) => value === "", {parseNumbers: true});


itaiad200 · 2022-05-14T20:03:22Z

webui/src/lib/components/repository/tree.jsx

    return (
        <Container className="m-4 mb-5">
            <h2 className="mt-2">To get started with this repository:</h2>

            <Row className="pt-2 ml-2">
                <DotIcon className="mr-1 mt-1"/>
-                <Button variant="link" onClick={onUpload}>Upload</Button>&nbsp;an object.
+                &nbsp;<Button variant="link" onClick={onImport}>Import</Button>&nbsp;data from the object store. Or, see the&nbsp;<a href="https://docs.lakefs.io/setup/import.html" target="_blank" rel="noopener noreferrer">docs</a>&nbsp;for other ways to import data to your repository.


How hard would it be to replace the object store from the actual object store name (e.g. S3, GCS, Azure) depending on the lakeFS type? I think it would make it much clearer.

itaiad200 · 2022-05-14T20:07:26Z

webui/src/pages/repositories/repository/objects.jsx

+                {(importState.inProgress) ? (<ImportProgress numObjects={importState.numObj} />)
+                    : ( <>
+                            <Alert variant="info">
+                                Importing  doesn&apos;t copy any data, it only create links to them in the lakeFS metadata. Don&apos;t worry, we will never make any changes to objects in the import source.


Suggested change

Importing doesn't copy any data, it only create links to them in the lakeFS metadata. Don't worry, we will never make any changes to objects in the import source.

Import doesn't copy the object. It only creates links to the objects in the lakeFS metadata layer. Don't worry, we will never change objects in the import source.

itaiad200 · 2022-05-14T20:11:13Z

webui/src/pages/repositories/repository/objects.jsx

+                            </Alert>
+                            <form>
+                            <Form.Group class='form-group'>
+                                <Form.Label><strong>Import source:</strong></Form.Label>


Suggested change

<Form.Label><strong>Import source:</strong></Form.Label>

<Form.Label><strong>Import from:</strong></Form.Label>

itaiad200 · 2022-05-14T20:14:37Z

webui/src/pages/repositories/repository/objects.jsx

+    const destRef = useRef(null);
+    const commitMsgRef = useRef(null);
+    const storageType = config.blockstore_type
+    const DEFAULT_BLOCKSTORE_VALIDITY_REGEX = new RegExp(`^s3://`);


Why? We support Azure and GCP too..

@eden-ohana @itaiad200 did we test this feature on Azure and GCS?

itaiad200 · 2022-05-14T20:25:16Z

webui/src/pages/repositories/repository/objects.jsx

+
+            try {
+                importBranchResp = await branches.get(repo.id, importBranch)
+            } catch (error){


Right, the import can also fail if there are uncommitted changes for example.

johnnyaug

Looking great!

import ui implementation

5fd1db5

eden-ohana added the include-changelog PR description should be included in next release changelog label May 11, 2022

eden-ohana requested a review from johnnyaug May 11, 2022 15:48

ImportDone styling

1d39a27

johnnyaug suggested changes May 11, 2022

View reviewed changes

johnnyaug reviewed May 12, 2022

View reviewed changes

webui/src/pages/repositories/repository/objects.jsx Outdated Show resolved Hide resolved

eden-ohana added 2 commits May 13, 2022 13:00

PR comments

762cd60

change import button icon

e71cee9

eden-ohana requested a review from johnnyaug May 13, 2022 11:32

johnnyaug suggested changes May 13, 2022

View reviewed changes

itaiad200 approved these changes May 14, 2022

View reviewed changes

eden-ohana and others added 4 commits May 16, 2022 13:40

PR comments

936f816

disable import button on local - empty repo

b442ff1

deleted default storage namespace

742cd8b

Merge branch 'master' into 3137/import-ui

00006fd

eden-ohana requested a review from johnnyaug May 16, 2022 11:23

eden-ohana added 3 commits May 16, 2022 14:44

validate source by config

b736aae

Merge remote-tracking branch 'origin/3137/import-ui' into 3137/import-ui

b1b2819

get branch - catch not found error

bb4b07b

johnnyaug approved these changes May 16, 2022

View reviewed changes

eden-ohana merged commit 7215af6 into master May 16, 2022

eden-ohana deleted the 3137/import-ui branch May 16, 2022 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

import UI implementation #3352

import UI implementation #3352

eden-ohana commented May 11, 2022

johnnyaug left a comment

johnnyaug May 11, 2022

eden-ohana May 13, 2022

johnnyaug commented May 12, 2022

eden-ohana commented May 13, 2022

johnnyaug left a comment

johnnyaug May 13, 2022

itaiad200 May 14, 2022

eden-ohana May 16, 2022

johnnyaug May 13, 2022

eden-ohana May 16, 2022

johnnyaug May 13, 2022

eden-ohana May 16, 2022

johnnyaug May 13, 2022

eden-ohana May 16, 2022

johnnyaug May 13, 2022

eden-ohana May 16, 2022

itaiad200 left a comment

itaiad200 May 14, 2022

eden-ohana May 16, 2022

itaiad200 May 14, 2022

itaiad200 May 14, 2022

eden-ohana May 16, 2022

itaiad200 May 14, 2022

eden-ohana May 16, 2022

itaiad200 May 14, 2022

eden-ohana May 16, 2022

itaiad200 May 14, 2022

ozkatz May 16, 2022

eden-ohana May 16, 2022

itaiad200 May 14, 2022

johnnyaug left a comment


		if (!reference \|\| reference.type !== RefTypeBranch) return <></>

		const checkStorageNamespaceValidity = () => {


		const UploadButton = ({ config, repo, reference, path, onDone, variant = "success", onClick, onHide, show = false}) => {
		const ImportButton = ({ config, repo, reference, path, onDone, onClick, variant = "success", onHide, show = false, enabled= false}) => {

		const requestURL = QueryString.stringifyUrl({url: `/repositories/${repoId}/branches/${branchId}/commits`, query: {source_metarange: source_metarange}});
		const parsedURL = QueryString.exclude(requestURL, (name, value) => value === "", {parseNumbers: true});

	Importing doesn't copy any data, it only create links to them in the lakeFS metadata. Don't worry, we will never make any changes to objects in the import source.
	Import doesn't copy the object. It only creates links to the objects in the lakeFS metadata layer. Don't worry, we will never change objects in the import source.

	<Form.Label><strong>Import source:</strong></Form.Label>
	<Form.Label><strong>Import from:</strong></Form.Label>

import UI implementation #3352

import UI implementation #3352

Conversation

eden-ohana commented May 11, 2022

johnnyaug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug commented May 12, 2022

eden-ohana commented May 13, 2022

johnnyaug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiad200 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug left a comment

Choose a reason for hiding this comment