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

fix: apply .bashrc when running a task #395

Merged
merged 3 commits into from
Aug 8, 2024
Merged

Conversation

vitaliy-guliy
Copy link
Contributor

@vitaliy-guliy vitaliy-guliy commented Aug 5, 2024

What does this PR do?

This is alternative to #383

What issues does this PR fix?

eclipse-che/che#23009

How to test this PR?

The variable seems to be different because of different .bashrs files in both containers.

Does this PR contain changes that override default upstream Code-OSS behavior?

  • the PR contains changes in the code folder (you can skip it if your changes are placed in a che extension )
  • the corresponding items were added to the CHANGELOG.md file
  • rules for automatic git rebase were added to the .rebase folder

Copy link

github-actions bot commented Aug 5, 2024

Click here to review and test in web IDE: Contribute

Copy link

github-actions bot commented Aug 5, 2024

@vitaliy-guliy vitaliy-guliy marked this pull request as ready for review August 6, 2024 10:01
@vitaliy-guliy
Copy link
Contributor Author

@AObuchow wdyt?

Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

👍

@@ -136,6 +138,11 @@ export class MachineExecClient implements vscode.Disposable {
* @returns a TerminalSession object to manage the created terminal session
*/
async createTerminalSession(component: string, commandLine?: string, workdir?: string, columns: number = 80, rows: number = 24): Promise<TerminalSession> {
if (commandLine) {
const bashrc = join(env.HOME!, '.bashrc');
commandLine = `test -f ${bashrc} >> /dev/null 2>&1 && source ${bashrc};${commandLine}`;
Copy link

Choose a reason for hiding this comment

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

It's worth creating a .bashrc that will cause an error (e.g. add cat nonExistentFile.txt) to see if that breaks running Devfile tasks with this approach. In theory, it shouldn't since we are using ; instead of &&. However, I suspect any errors in calling source ~/.bashrc will be logged when running the Devfile task. I'm not sure if that is desired?

A potential advantage of #383 is that we only call source ~/.bashrc once, and catch the error.

In the current approach, if the call to source ~/.bashrc results in any errors, the errors will repeatedly be hit when running a devfile task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea to experiment with .bashrc. Let me then update the repository I'm using to test the pull request.

However, I suspect any errors in calling source ~/.bashrc will be logged when running the Devfile task. I'm not sure if that is desired?

Good catch. I will experiment with this as well.

@vitaliy-guliy
Copy link
Contributor Author

I found another mistaking.
HOME in mongo container is configured to /var/lib/mongodb. But the path to .bashrc file was taken based on the HOME taken from the tools container.

const bashrc = join(env.HOME!, '.bashrc');

As result .bashrc from mounted /home/user directory will be used as a source.
This is not right.
It needs to get HOME value from mongo container and try to find .bashrc based on the given value.

@AObuchow
Copy link

AObuchow commented Aug 6, 2024

I found another mistaking. HOME in mongo container is configured to /var/lib/mongodb. But the path to .bashrc file was taken based on the HOME taken from the tools container.

const bashrc = join(env.HOME!, '.bashrc');

As result .bashrc from mounted /home/user directory will be used as a source. This is not right. It needs to get HOME value from mongo container and try to find .bashrc based on the given value.

Isin't that the intended behaviour for resolving eclipse-che/che#23009 however? I thought we wanted the same $PATH in other containers that the tooling has?

@vitaliy-guliy
Copy link
Contributor Author

Isin't that the intended behaviour for resolving eclipse-che/che#23009 however? I thought we wanted the same $PATH in other containers that the tooling has?

If the container has its own .bashrc, lets use it.
And it does not prevent us from resolving eclipse-che/che#23009

Signed-off-by: vitaliy-guliy <[email protected]>
@vitaliy-guliy vitaliy-guliy marked this pull request as draft August 6, 2024 20:25
Signed-off-by: vitaliy-guliy <[email protected]>
Copy link

github-actions bot commented Aug 7, 2024

@vitaliy-guliy
Copy link
Contributor Author

There is no .bashrc file in the home directory in mongo container.

To test the behavior I created a .bashrc file in project directory and added a test variable.
Then I opened a terminal in mongo container and copied the .bashrc to HOME directory with

$ cp /projects/vscode-test-extension/.bashrc ${HOME}/

and then checked for the file

$ ls -la ${HOME}
total 12
drwxrwxr-x. 1 mongodb    root         50 Aug  7 13:47 .
drwxr-xr-x. 1 root       root         21 Sep 17  2020 ..
-rw-r--r--. 1 1000300000 root         31 Aug  7 13:47 .bashrc
-rw-------. 1 1000300000 root        111 Aug  7 13:33 .dbshell
drwxr-xr-x. 2 1000300000 root         20 Aug  7 13:33 .kube
drwxrwsr-x. 4 root       1000300000 4096 Aug  7 13:39 data

After that I launched Run test script - mongo task and checked the corresponding .bashrc is applied

Screenshot from 2024-08-07 16-48-40

@vitaliy-guliy vitaliy-guliy marked this pull request as ready for review August 7, 2024 13:53
@vitaliy-guliy vitaliy-guliy mentioned this pull request Aug 7, 2024
3 tasks
Copy link

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@vitaliy-guliy vitaliy-guliy merged commit dd29c50 into main Aug 8, 2024
7 checks passed
@vitaliy-guliy vitaliy-guliy deleted the bash-as-a-source-2 branch August 8, 2024 12:04
@devstudio-release
Copy link

Build 3.17 :: code_3.x/1450: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.16 :: code_3.16/1: Console, Changes, Git Data

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.16 :: update-digests_3.16/10: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.16 :: code_3.16/1: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.16/6 triggered

@devstudio-release
Copy link

Build 3.16 :: operator-bundle_3.16/5: Console, Changes, Git Data

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.17 :: code_3.x/1450: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/7396 triggered

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.17 :: get-sources-rhpkg-container-build_3.x/7384: FAILURE

devspaces-operator-bundle : 3.x :: Failed in 63219318 : BREW:BUILD/STATUS:UNKNOWN
FAILURE:; copied to quay

@devstudio-release
Copy link

Build 3.16 :: copyIIBsToQuay/2748: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.16 :: sync-to-downstream_3.16/11: SUCCESS

Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.16/11 triggered; /job/DS_CI/job/dsc_3.16 triggered;

@devstudio-release
Copy link

Build 3.16 :: operator-bundle_3.16/5: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.16/11 triggered

@devstudio-release
Copy link

Build 3.16 :: dsc_3.16/6: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.16 :: update-digests_3.16/10: SUCCESS

Detected new images: rebuild operator-bundle
* code; /DS_CI/operator-bundle_3.16/5 triggered

@devstudio-release
Copy link

Build 3.16 :: dsc_3.16/6: SUCCESS

3.16.0-CI

@devstudio-release
Copy link

Build 3.16 :: copyIIBsToQuay/2748: SUCCESS

3.16
arches = x86_64, s390x, ppc64le;
  * LATEST DS OPERATOR BUNDLE = <a href=https://quay.io/repository/devspaces/devspaces-operator-bundle?tab=tags>registry-proxy.engineering.redhat.com/rh-osbs/devspaces-operator-bundle:3.16-43
  * LATEST DWO OPERATOR BUNDLE = <a href=https://quay.io/repository/devworkspace/devworkspace-operator-bundle?tab=tags>registry-proxy.engineering.redhat.com/rh-osbs/devworkspace-operator-bundle:???
+ s390x-rhel8 IIB(s) copied:
  + quay.io/devspaces/iib:3.16-v4.16-785039-s390x
  + quay.io/devspaces/iib:3.16-v4.16-s390x
  + quay.io/devspaces/iib:latest-v4.16-s390x
  + quay.io/devspaces/iib:3.16-v4.15-785221-s390x
  + quay.io/devspaces/iib:3.16-v4.15-s390x
  + quay.io/devspaces/iib:latest-v4.15-s390x
  + quay.io/devspaces/iib:3.16-v4.14-785220-s390x
  + quay.io/devspaces/iib:3.16-v4.14-s390x
  + quay.io/devspaces/iib:latest-v4.14-s390x
  + quay.io/devspaces/iib:3.16-v4.13-785219-s390x
  + quay.io/devspaces/iib:3.16-v4.13-s390x
  + quay.io/devspaces/iib:latest-v4.13-s390x
  + quay.io/devspaces/iib:3.16-v4.12-785224-s390x
  + quay.io/devspaces/iib:3.16-v4.12-s390x
  + quay.io/devspaces/iib:latest-v4.12-s390x
+ x86_64-rhel8 IIB(s) copied:
  + quay.io/devspaces/iib:3.16-v4.16-785039-x86_64
  + quay.io/devspaces/iib:3.16-v4.16-x86_64
  + quay.io/devspaces/iib:latest-v4.16-x86_64
  + quay.io/devspaces/iib:3.16-v4.15-785221-x86_64
  + quay.io/devspaces/iib:3.16-v4.15-x86_64
  + quay.io/devspaces/iib:latest-v4.15-x86_64
  + quay.io/devspaces/iib:3.16-v4.14-785220-x86_64
  + quay.io/devspaces/iib:3.16-v4.14-x86_64
  + quay.io/devspaces/iib:latest-v4.14-x86_64
  + quay.io/devspaces/iib:3.16-v4.13-785219-x86_64
  + quay.io/devspaces/iib:3.16-v4.13-x86_64
  + quay.io/devspaces/iib:latest-v4.13-x86_64
  + quay.io/devspaces/iib:3.16-v4.12-785224-x86_64
  + quay.io/devspaces/iib:3.16-v4.12-x86_64
  + quay.io/devspaces/iib:latest-v4.12-x86_64
+ ppc64le-rhel8 IIB(s) copied:
  + quay.io/devspaces/iib:3.16-v4.16-785039-ppc64le
  + quay.io/devspaces/iib:3.16-v4.16-ppc64le
  + quay.io/devspaces/iib:latest-v4.16-ppc64le
  + quay.io/devspaces/iib:3.16-v4.15-785221-ppc64le
  + quay.io/devspaces/iib:3.16-v4.15-ppc64le
  + quay.io/devspaces/iib:latest-v4.15-ppc64le
  + quay.io/devspaces/iib:3.16-v4.14-785220-ppc64le
  + quay.io/devspaces/iib:3.16-v4.14-ppc64le
  + quay.io/devspaces/iib:latest-v4.14-ppc64le
  + quay.io/devspaces/iib:3.16-v4.13-785219-ppc64le
  + quay.io/devspaces/iib:3.16-v4.13-ppc64le
  + quay.io/devspaces/iib:latest-v4.13-ppc64le
  + quay.io/devspaces/iib:3.16-v4.12-785224-ppc64le
  + quay.io/devspaces/iib:3.16-v4.12-ppc64le
  + quay.io/devspaces/iib:latest-v4.12-ppc64le

@vitaliy-guliy vitaliy-guliy added the made-with-che Changes made with Che-Code label Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
made-with-che Changes made with Che-Code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants