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.

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

Sharable Kedro-Viz UI for AWS, GCP and Azure #1718

Merged
merged 26 commits into from
Feb 22, 2024

Conversation

jitu5
Copy link
Contributor

@jitu5 jitu5 commented Jan 25, 2024

Description

Resolves #1674 & #1675

Sharable URL modal UI updated with below changes

Screenshot 2024-01-25 at 3 11 02 p m Screenshot 2024-01-25 at 3 10 50 p m

Development notes

  • shareable-url-modal component updated as per new design
  • shareable-url-modal e2e cypress test updated
  • In metadata.js and settings-modal.js version info and experiment tracking link now conditionally hidden
  • <Input> component updated to accommodate input and textarea type
  • <Dropdown> component updated to accommodate “placehoder” text
  • <Modal> component updated to make title conditional

QA notes

Note: This will be merged into the branch feature/shareableviz-extended-support.
Deploying to AWS, GCP and Azure will only works from UI after 1708 & 1711 get merged into the branch feature/shareableviz-extended-support

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

@jitu5 jitu5 self-assigned this Jan 25, 2024
@jitu5 jitu5 marked this pull request as ready for review January 30, 2024 10:17
@jitu5 jitu5 requested a review from tynandebold as a code owner January 30, 2024 10:17
@jitu5 jitu5 requested review from tynandebold and removed request for tynandebold January 30, 2024 10:17
@jitu5 jitu5 added the Javascript Pull requests that update Javascript code label Jan 30, 2024
};

const handleResponseUrl = () => {
// If the URL does not start with http:// or https://, append http:// to avoid relative path issue
Copy link
Contributor

Choose a reason for hiding this comment

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

appending http will not work for Azure. By default AzureBlobStorage serves content on https
Secure transfer required is enabled by default

image

return url;
}
return responseUrl;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how to handle this if the user enters a wrong endpoint url . There can be many cases of url not entered properly. We should document this in our docs and avoid handling all cases. I think you did this to handle gcp load balancer usecase @jitu5 ?

Copy link
Contributor Author

@jitu5 jitu5 Feb 21, 2024

Choose a reason for hiding this comment

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

In the case of GCP with a load balancer, the endpoint will most likely be an IP address, such as 32.206.61.13:80. If a user pastes this IP address into the endpoint input field, the clickable link on the success page becomes relative to the current address, like https://example.com/32.206.61.13:80. To address this, I have appended 'http://' to the link.

Should this handling be specific to GCP @ravi-kumar-pilla ?

@ravi-kumar-pilla
Copy link
Contributor

Hi Team,

We also need to handle the case where the credentials are configured correctly but the bucket_name is incorrect. Below is the output and error type as of now -

AWS
BE Error - FileNotFoundError: The specified bucket does not exist

FE -

Screenshot 2024-02-20 at 3 38 19 PM

Azure
BE Error - ServiceRequestError: Cannot connect to host somek.blob.core.windows.net:443 ssl:default [nodename nor servname provided, or not known]

FE -

Screenshot 2024-02-20 at 3 37 45 PM

GCP
BE Error - FileNotFoundError: https://storage.googleapis.com/upload/storage/v1/b/somek/o

FE -

Publish and Share Kedro-Viz

Should we create a separate ticket and handle these scenarios by passing a common error message like - The specified bucket does not exist when the BE error is either FileNotFoundError or ServiceRequestError ? @rashidakanchwala

Thank you

@rashidakanchwala
Copy link
Contributor

rashidakanchwala commented Feb 21, 2024

Hi Ravi, I also agree the above errors are quite vague. As a short term solution -- let's have a common - 'The specified bucket does not exist' for FileNotFound or ServiceRequestError.

But let's also create a seperate ticket to handle error mesages as we will have to think through this a bit more. Once Steph is back, we can map out different success/failure journeys.

@ravi-kumar-pilla
Copy link
Contributor

Hi Ravi, I also agree the above errors are quite vague. As a short term solution -- let's have a common - 'The specified bucket does not exist' for FileNotFound or ServiceRequestError.

Sure will add it in the current branch.

But let's also create a seperate ticket to handle error mesages as we will have to think through this a bit more. Once Steph is back, we can map out different success/failure journeys.

Sure, will do that

@ravi-kumar-pilla
Copy link
Contributor

Hi Team,

This might be a design question -> when we publish successfully and click on link settings -> The publish button text displays as Republish. This is great but if we change the hosting platform or change the bucket_name, will the new publishing be a re-publish or just publish ?

image

I feel we should stick to the text as Publish in both the cases. Thank you

Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla left a comment

Choose a reason for hiding this comment

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

Awesome work ! Looks good to me. I have left a design comment which is not a blocker. Thank you @jitu5 for the PR

@rashidakanchwala
Copy link
Contributor

Hi Team,

This might be a design question -> when we publish successfully and click on link settings -> The publish button text displays as Republish. This is great but if we change the hosting platform or change the bucket_name, will the new publishing be a re-publish or just publish ?

image I feel we should stick to the text as **Publish** in both the cases. Thank you

You're correct. I also found 'Republish' a bit confusing. @stephkaiser's original design involved guiding users through multiple selections. Initially, you choose the 'cloud provider,' then click 'Next' to proceed to the form where you enter details. However, upon returning to 'Link Settings,' it directs you straight to the form page without an option to switch 'Cloud Provider.' We diverged from Steph's original design because it assumed each Cloud Provider would have different input fields. However, since they're now the same, the two-step process feels unnecessarily lengthy.

When Steph returns, perhaps we can refine this journey. In the meantime, I propose that when the user clicks 'Link Settings', the dropdown for selecting the cloud provider should be disabled. This adjustment would make 'Republish' more intuitive. Otherwise, we should revert to using 'Publish.'

Signed-off-by: <>
@jitu5
Copy link
Contributor Author

jitu5 commented Feb 22, 2024

@rashidakanchwala @ravi-kumar-pilla For simplification I am keeping the text as Publish in both the cases.

@rashidakanchwala
Copy link
Contributor

Amazing, LGTM. thanks Jitendra!! <3

@rashidakanchwala rashidakanchwala self-requested a review February 22, 2024 15:00
@jitu5 jitu5 merged commit db885b4 into feature/shareableviz-extended-support Feb 22, 2024
17 checks passed
@jitu5 jitu5 deleted the feature/share_viz_ui branch February 22, 2024 15:41
ravi-kumar-pilla added a commit that referenced this pull request Feb 26, 2024
* refactor router to accept new deployer inputs (#1739)

Signed-off-by: ravi-kumar-pilla <[email protected]>

* Refactor Shareableviz CLI (#1740)

* refactor router to accept new deployer inputs

Signed-off-by: ravi-kumar-pilla <[email protected]>

* refactor cli for shareableviz deploy

Signed-off-by: ravi-kumar-pilla <[email protected]>

* merge router change

Signed-off-by: ravi-kumar-pilla <[email protected]>

* PR comments fix

Signed-off-by: ravi-kumar-pilla <[email protected]>

---------

Signed-off-by: ravi-kumar-pilla <[email protected]>

* Kedro Viz Static Website hosting on Azure  (#1708)

* CLI command kedro viz build added

* Lint fix

* lint fix

* Lint fix

* add mypy ignore

* Missing build file added

* Lint error fix

* BaseDeployer class added

* Unused code removed

* Fix lint issue

* azure deploy initial draft

Signed-off-by: ravi-kumar-pilla <[email protected]>

* added base_deployer

* add deployer factory

* partial working draft

Signed-off-by: ravi-kumar-pilla <[email protected]>

* Test and comments of deployers updated

* test draft

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix lint

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove circular dependency

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix lint

Signed-off-by: ravi-kumar-pilla <[email protected]>

* revert back consent

Signed-off-by: ravi-kumar-pilla <[email protected]>

* minor updates

Signed-off-by: ravi-kumar-pilla <[email protected]>

* update pytests

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add pytest for azure shareableviz

Signed-off-by: ravi-kumar-pilla <[email protected]>

* refactor and add timeout

Signed-off-by: ravi-kumar-pilla <[email protected]>

* refactor cli

Signed-off-by: ravi-kumar-pilla <[email protected]>

* update pytest

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add release note

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix PR comments and flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* testing flaky c
y test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* resolve conflicts

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix PR comments

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add back cypress flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove cypress flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove duplicate pytest parameter

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove fsspec upper bound

Signed-off-by: ravi-kumar-pilla <[email protected]>

---------

Signed-off-by: ravi-kumar-pilla <[email protected]>
Co-authored-by: Jitendra Gundaniya <[email protected]>
Co-authored-by: rashidakanchwala <[email protected]>

* Kedro Viz Static Website Hosting on GCP (#1711)

* CLI command kedro viz build added

* Lint fix

* lint fix

* Lint fix

* add mypy ignore

* Missing build file added

* Lint error fix

* BaseDeployer class added

* Unused code removed

* Fix lint issue

* azure deploy initial draft

Signed-off-by: ravi-kumar-pilla <[email protected]>

* added base_deployer

* add deployer factory

* partial working draft

Signed-off-by: ravi-kumar-pilla <[email protected]>

* Test and comments of deployers updated

* test draft

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix lint

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove circular dependency

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix lint

Signed-off-by: ravi-kumar-pilla <[email protected]>

* revert back consent

Signed-off-by: ravi-kumar-pilla <[email protected]>

* initial draft

Signed-off-by: ravi-kumar-pilla <[email protected]>

* minor updates

Signed-off-by: ravi-kumar-pilla <[email protected]>

* update pytests

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add pytest for azure shareableviz

Signed-off-by: ravi-kumar-pilla <[email protected]>

* refactor and add timeout

Signed-off-by: ravi-kumar-pilla <[email protected]>

* refactor cli

Signed-off-by: ravi-kumar-pilla <[email protected]>

* update pytest

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add release note

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix PR comments and flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* testing flaky c
y test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add pytest for gcp

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix gcp pytest coverage

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix lint

Signed-off-by: ravi-kumar-pilla <[email protected]>

* update pytest

Signed-off-by: ravi-kumar-pilla <[email protected]>

* revert file permission change

---------

Signed-off-by: ravi-kumar-pilla <[email protected]>
Co-authored-by: Jitendra Gundaniya <[email protected]>
Co-authored-by: rashidakanchwala <[email protected]>

* Sharable Kedro-Viz UI for AWS, GCP and Azure (#1718)

* shareable viz with multiple platform UI

* test fix

* Relative path fix for user entered url

* Test updated

* Test updated

* UI alignment and endpoint help text added.

Signed-off-by: <>

* Add Kedro-Viz documentation URLs and update modal content

Signed-off-by: <>

* Code review suggestions added

Signed-off-by: <>

* Refactor form field names in ShareableUrlModal component

Signed-off-by: <>

* Add cursor style to input field in shareable-url-modal.js

Signed-off-by: <>

* Add cursor style to input field in shareable-url-modal.js

Signed-off-by: <>

* Variable names updated.

Signed-off-by: <>

* add exception handlers

Signed-off-by: ravi-kumar-pilla <[email protected]>

* Disclaimer note added and shareable-url-modal component refactored by moving major jsx to other file

Signed-off-by: <>

* Wrapped the prop methods with useCallback

Signed-off-by: <>

* info text updated.

Signed-off-by: <>

* Test fixed

Signed-off-by: <>

* test fix and rearranging jsx

Signed-off-by: <>

* Cypress test fix

Signed-off-by: <>

* button text updated

Signed-off-by: <>

* Keeping button text as Publish for a link seetings

Signed-off-by: <>

---------

Signed-off-by: <>
Signed-off-by: ravi-kumar-pilla <[email protected]>
Co-authored-by: ravi-kumar-pilla <[email protected]>

* Kedro-viz doc link added in disclaimer note.

Signed-off-by: <>

---------

Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: <>
Co-authored-by: Jitendra Gundaniya <[email protected]>
Co-authored-by: rashidakanchwala <[email protected]>
Co-authored-by: Jitendra Gundaniya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants