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

Add /QOpenSys/pkgs/bin to .bashrc if needed #1695

Merged
merged 5 commits into from
Dec 12, 2023
Merged

Conversation

sebjulliand
Copy link
Collaborator

Changes

Resolves #1691

This PR enhances the connection process and make it look for /QOpenSys/pkgs/bin in the PATH environment variable.
If it's not there, it will offer to either create ~/.bashrc file or modify it to append /QOpenSys/pkgs/bin to the PATH environment variable.

The creation/modification is delayed in the connection process, so it's only done once the connection is ready to do it.

The connection process will only check this once or if the user connects and reload the server settings, so we won't bother someone who would not like to modify its PATH.

Since .bashrc is only relevant for non-login shell, this shouldn't interfere with .profile or .bash_profile.

Checklist

  • have tested my change
  • for feature PRs: PR only includes one feature enhancement.

Signed-off-by: Seb Julliand <sebjulliand@gmail.com>
@sebjulliand sebjulliand added the enhancement New feature or request label Dec 7, 2023
@sebjulliand sebjulliand requested a review from a team December 7, 2023 15:08
Signed-off-by: Seb Julliand <sebjulliand@gmail.com>
@worksofliam worksofliam requested a review from ThePrez December 7, 2023 15:11
@chrjorgensen
Copy link
Collaborator

@sebjulliand Questions:

  1. Is your code always run - or only run when the user shell is bash?
  2. There is a test in the connection for bash as shell - is this code linked to that check? If not, maybe it should?

@sebjulliand
Copy link
Collaborator Author

@sebjulliand Questions:

1. Is your code always run - or only run when the user shell is `bash`?

2. There is a test in the connection for `bash` as shell - is this code linked to that check? If not, maybe it should?

Good questions! The code runs whatever shell is used and do not run only if bash is the current shell. On the other hand, we expect the shell to be bash and offer to change it if it's not; that's why I didn't think about conditioning this action to bash only.

What do you think?

@chrjorgensen
Copy link
Collaborator

we expect the shell to be bash

Do we expect it? It's not a requirement (to my knowledge), although it could be argued that it should be... however, the users may use the extension against older or customer systems, where bash or OSS environment is not installed.

My thoughts are that we should keep the bash checking code together and not have code in different places changing user settings.

Additional comment about PATH: /QOpenSys/pkgs/bin should be before /usr/bin and /QOpenSys/usr/bin - but you don't check for that. Would it be easy to have this check too?

@sebjulliand
Copy link
Collaborator Author

we expect the shell to be bash

Do we expect it? It's not a requirement (to my knowledge), although it could be argued that it should be... however, the users may use the extension against older or customer systems, where bash or OSS environment is not installed.

My thoughts are that we should keep the bash checking code together and not have code in different places changing user settings.

Fair enough! We'll only do this if the current shell is bash then.

Additional comment about PATH: /QOpenSys/pkgs/bin should be before /usr/bin and /QOpenSys/usr/bin - but you don't check for that. Would it be easy to have this check too?

I don't check it but since I always add /QOpenSys/pkgs/bin in first place in the path, that should not be necessary 😄

@chrjorgensen
Copy link
Collaborator

chrjorgensen commented Dec 8, 2023

I don't check it but since I always add /QOpenSys/pkgs/bin in first place in the path, that should not be necessary

But your check will accept a PATH having /QOpenSys/pkgs/bin after the other ones - which is not correct (maybe a user mistake). 😉

(Sorry for being such a P... in the A..! 😆 )

@sebjulliand
Copy link
Collaborator Author

I don't check it but since I always add /QOpenSys/pkgs/bin in first place in the path, that should not be necessary

But your check will accept a PATH having /QOpenSys/pkgs/bin after the other ones - which is not correct (maybe a user mistake). 😉

(Sorry for being such a P... in the A..! 😆 )

No, no, you're absolutely right 😅

Signed-off-by: Seb Julliand <sebjulliand@gmail.com>
@sebjulliand
Copy link
Collaborator Author

@chrjorgensen I followed your advice and:

  • Only run this whole process is bash is the current shell
  • Check if /QOpenSys/pkgs/bin appears before anything else in PATH

Hopefully this will receive the "Jørgensen Seal of Approval" 😁

@worksofliam worksofliam added this to the 2.6.0 milestone Dec 11, 2023
src/api/IBMi.ts Outdated Show resolved Hide resolved
src/api/IBMi.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@worksofliam worksofliam left a comment

Choose a reason for hiding this comment

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

Left some comments. I think the console.log should be removed.

Signed-off-by: Seb Julliand <sjulliand@arcadsoftware.com>
Namely if it appears after /usr/bin or /QOpenSys/usr/bin

Signed-off-by: Seb Julliand <sjulliand@arcadsoftware.com>
Copy link
Contributor

@worksofliam worksofliam left a comment

Choose a reason for hiding this comment

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

Looks like it works correctly for me!

image image

(I had to connect and reload as expected to test the positional case)

@sebjulliand sebjulliand merged commit 92c91f7 into master Dec 12, 2023
1 check passed
@sebjulliand sebjulliand deleted the feature/autoBashrc branch December 12, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make bash files automatically
3 participants