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

vdk-jupyter: run VDK job #1454

Merged
merged 17 commits into from
Dec 22, 2022
Merged

vdk-jupyter: run VDK job #1454

merged 17 commits into from
Dec 22, 2022

Conversation

duyguHsnHsn
Copy link
Collaborator

This change introduces a new feature to the Jupyterlab extension. It allows users to run VDK job from the Jupyter UI.
More information on how to work with the command can be found in UX Flows section of the Jupyter VEP.
Also it solves the issue since the connection is working now.
Signed-off-by: Duygu Hasan [email protected]

murphp15
murphp15 approved these changes Dec 19, 2022
View reviewed changes
Copy link
Collaborator

@murphp15 murphp15 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

ivakoleva
ivakoleva approved these changes Dec 19, 2022
View reviewed changes
Copy link
Contributor

@ivakoleva ivakoleva left a comment

Choose a reason for hiding this comment

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

LGTM, I think we may resolve the codacy issues and merge

@duyguHsnHsn duyguHsnHsn merged commit 32ba164 into main Dec 22, 2022
@duyguHsnHsn duyguHsnHsn deleted the person/hduygu/jupyterlab-vdk-run-job branch December 22, 2022 11:47
antoniivanov
antoniivanov reviewed Dec 22, 2022
View reviewed changes
projects/vdk-plugins/vdk-jupyter/vdk-jupyterlab-extension/vdk-jupyterlab-extension/vdk.py
process = subprocess.Popen(
cmd, stdout=log_file, stderr=log_file, env=os.environ.copy()
)
subprocess.check_output(["/bin/ls", "-l"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was just for testing purposes I assume. But it should not have been merged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shall I remove it now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is a good suggestion https://stackoverflow.com/questions/21009416/python-subprocess-security

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoniivanov antoniivanov antoniivanov approved these changes

@murphp15 murphp15 murphp15 approved these changes

@ivakoleva ivakoleva ivakoleva approved these changes

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@duyguHsnHsn @antoniivanov @murphp15 @ivakoleva @vmwclabot