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

vdk-jupyter: automate create options #2506

Merged
merged 9 commits into from
Aug 9, 2023

Conversation

duyguHsnHsn
Copy link
Collaborator

@duyguHsnHsn duyguHsnHsn commented Aug 1, 2023

What:
Changed create job option to create local job automatically and cloud if REST_API_URL is set.

Currently there are two options for created job result:
Created both on cloud and locally:
Screenshot 2023-08-08 at 14 46 03

Created locally only because of lack of url:
Screenshot 2023-08-08 at 14 42 32
Screenshot 2023-08-08 at 14 42 42

The dialog for create now looks like this:
Screenshot 2023-08-04 at 12 18 13

Why:
Currently, when the user wants to create a job from the Jupyter UI he gets dialog asking whether it will be a local job, a cloud job or both. We discussed and got on an agreement that the options should be removed from the dialog.

Creating job should be:

  • always creating locally
  • creating in the cloud only if REST_API_URL is set

Signed-off-by: Duygu Hasan hduygu@vmware.com

@duyguHsnHsn duyguHsnHsn marked this pull request as ready for review August 1, 2023 11:43
Copy link
Contributor

@DeltaMichael DeltaMichael left a comment

Choose a reason for hiding this comment

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

I disagree with the concept. This is not a good user experience. What if I want to create a job just locally, but I was creating cloud jobs previously. Do I have to unset the env variable and then set it again to get back to what I was doing?

I wasn't part of the discussions, however, which is completely my fault. I'll approve and we can possibly raise it again down the line.

@antoniivanov
Copy link
Collaborator

antoniivanov commented Aug 3, 2023

I disagree with the concept. This is not a good user experience. What if I want to create a job just locally, but I was creating cloud jobs previously. Do I have to unset the env variable and then set it again to get back to what I was doing?

I wasn't part of the discussions, however, which is completely my fault. I'll approve and we can possibly raise it again down the line.

The concept of local and cloud doesn't really exist in the user's mind. It's something we are forcing on them. And we should not really. The fewer things the user needs to know and understand the better their experience would be.

As far as the user is concerned a job is a job. They want to be able to create it and develop it and when it's ready schedule it /deploy it so it runs regularly and monitor.

The less things you make the user think about the better their life will be. Because they do not know (nor need to care about the concept of local vs cloud), they would not really care if job is created in the cloud (by created we really mean the name is reserved in most cases).

This is also consistent with the way CLI works (see __determine_cloud_local_flags )

@DeltaMichael
Copy link
Contributor

DeltaMichael commented Aug 4, 2023

The less things you make the user think about the better their life will be. Because they do not know (nor need to care about the concept of local vs cloud), they would not really care if job is created in the cloud (by created we really mean the name is reserved in most cases).

@antoniivanov

I disagree, because we're actually making the user think about more things. We're making them think about an environment variable that also has a side effect when set. It determines both the control service URL and if we talk to the control service.

Let's say the user really doesn't need to know about that. Then they'll create a bunch of local jobs they have no intention of creating and deploying in the cloud, look at their cloud environment UI and discover that it's filled with garbage entries. Then they'll wonder why and will have to think about the env variable. It's quite a confusing user experience.

Now compare that to a checkbox in the UI that says "Cloud".

This is not a new problem, btw. I once spent a good chunk of time trying to change some configs in a test data job and not understanding why my deployments were still failing. Then I found out about the .vdk.internal dot folder by accident and it all made sense. It's barely documented https://github.com/vmware/versatile-data-kit/blob/main/projects/vdk-control-cli/README.md?plain=1#L26 and I assume it's due to similar reasons.

Sorry for going off on a tangent, maybe we can have this discussion further down the line. 😇

@duyguHsnHsn
Copy link
Collaborator Author

We decided to keep it as it is proposed in the PR for the MVP.
This might be changed or kept for the Beta version of the Integration.
check #2535

Copy link
Collaborator

@antoniivanov antoniivanov left a comment

Choose a reason for hiding this comment

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

How do we test the workflow ? Doesn't make sense to add test to ui-tests/ ?

Otherwise looks good to me

@duyguHsnHsn duyguHsnHsn force-pushed the person/hduygu/vdk-jupyter-automate-create-options branch from fe10956 to 81b90e6 Compare August 9, 2023 10:36
@duyguHsnHsn duyguHsnHsn merged commit f363c7f into main Aug 9, 2023
7 of 8 checks passed
@duyguHsnHsn duyguHsnHsn deleted the person/hduygu/vdk-jupyter-automate-create-options branch August 9, 2023 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants