Skip to content

Conversation

sleshchenko
Copy link
Member

@sleshchenko sleshchenko commented Mar 29, 2021

What does this PR do?

Add volume into custom web terminal sample.

What issues does this PR fix or reference?

N/A

Is it tested? How?

Made sure that the sample work fine with the latest tooling image after redhat-developer/web-terminal-tooling#26 is merged.

It did not work for me with default latest WTO, so a dedicated issue is created https://issues.redhat.com/browse/WTO-89

@openshift-ci-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Collaborator

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

This may cause a few other issues. Looking at the contents of the web terminal tooling container, without the entrypoint, I see

$ ls -al $HOME
total 20
drwxrwxr-x. 2 root root 4096 Feb 25 15:39 .
drwxr-xr-x. 1 root root 4096 Feb 25 15:38 ..
-rw-rw-r--. 1 root root   77 Feb 25 15:39 .bashrc
-rw-rw----. 1 root root  533 Feb 25 15:39 .viminfo

where .bashrc contains some completions. Mounting a volume into this path will wipe out these files.

We should update the web-terminal-tooling image to accomodate a mounted $HOME dir (e.g. by putting completions /tmp/home and copying over to the PVC on start.

I'm not sure why, but with the current PR, the /home/user directory is being created by root and not the running user 🤔

- name: PS1
value: \[\e[34m\]>\[\e[m\]\[\e[33m\]>\[\e[m\]
volumeMounts:
- path: "/home/"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we mount to /home/user?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to /home/user.

but actually, it's strange that I got permissions denied error when I try /home, somebody creates non-writable /home/user.

@amisevsk
Copy link
Collaborator

Created https://issues.redhat.com/browse/WTO-82 to track updates needed for the tooling image.

@openshift-ci
Copy link

openshift-ci bot commented Apr 12, 2021

@sleshchenko: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/v7-devworkspace-happy-path 1a3eaa8 link /test v7-devworkspace-happy-path

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@amisevsk
Copy link
Collaborator

PR redhat-developer/web-terminal-tooling#26 is merged so this should be good to go as soon as the change is propagated into the image.

@sleshchenko
Copy link
Member Author

sleshchenko commented Apr 14, 2021

@amisevsk I tried the sample with the latest manually built tooling (quay.io/sleshche/web-terminal-tooling:latest) and it does not seem to work as expected:
Screenshot_20210414_125828

> kc logs workspace2ca8278ee0e3489f-7b6f75765d-dj2tn -c tooling 
cp: cannot create regular file '/home/user/.bashrc': Permission denied

Update:
it was the default WTO from https://github.com/redhat-developer/web-terminal-operator (make install)
The DWO from main branch works file.

@sleshchenko sleshchenko marked this pull request as ready for review April 14, 2021 10:39
@amisevsk
Copy link
Collaborator

/test all

Copy link
Collaborator

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, sleshchenko

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [amisevsk,sleshchenko]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sleshchenko
Copy link
Member Author

/test all

does nothing 😕
I tried to patch triggers but they are generated in not the best way 🤷‍♂️

@sleshchenko sleshchenko merged commit 78be464 into devfile:main Apr 15, 2021
@sleshchenko sleshchenko deleted the terminalWithVolume branch April 15, 2021 09:59
@amisevsk
Copy link
Collaborator

/test all

does nothing
I tried to patch triggers but they are generated in not the best way

The bot LIES:

Use /test all to run the following jobs:

  • pull-ci-devfile-devworkspace-operator-main-v7-images

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.

3 participants