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: Pick up REST API URL from the env #2142

Merged
merged 5 commits into from
May 30, 2023

Conversation

gageorgiev
Copy link
Contributor

@gageorgiev gageorgiev commented May 29, 2023

Currently, users are expected to configure the REST API URL manually, which is a bad practice. After this change, the REST API URL will be picked up from the environment, which means it will be configured when deploying the docker image.
Also, a file with a typo in its name was renamed.

Testing done: pipelines

Currently, users are expected to configure the REST API URL
manually, which is a bad practice. After this change, the
REST API URL will be picked up from the environment, which
means it will be configured when deploying the docker image.

Testing done: pipelines

Signed-off-by: Gabriel Georgiev <gageorgiev@vmware.com>
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.

Thanks for doing this change.

This would improve security and maintainability vdk-jypyter. Using environment variables to configure deployment-specific details like the REST API URL is a common practice and allows for more flexibility and easier management.

@duyguHsnHsn
Copy link
Collaborator

Also see jobData.ts, vdkOptions.py, and vdkOptions.ts. Rest api url options should be removed from there, as well.

Signed-off-by: Gabriel Georgiev <gageorgiev@vmware.com>
Signed-off-by: Gabriel Georgiev <gageorgiev@vmware.com>
Signed-off-by: Gabriel Georgiev <gageorgiev@vmware.com>
Copy link
Collaborator

@duyguHsnHsn duyguHsnHsn left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@yonitoo yonitoo left a comment

Choose a reason for hiding this comment

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

LGTM

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.

One more thing. Almost never use raise Exception. If you see it in the code, we should fix it.

@gageorgiev gageorgiev merged commit e4f392c into main May 30, 2023
11 checks passed
@gageorgiev gageorgiev deleted the person/gageorgiev/jupyter-rest-url branch May 30, 2023 11:30
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

6 participants