refactor (shell) : detect shell on windows using gopsutil too (#4588) #4591

rohanKanojia · 2025-02-03T05:45:13Z

Description

As a follow up to #4572, updating shell detection logic on windows to also rely on gopsutil library to detect the shell name by inspecting parent processes of crc.exe process.

Fixes: #4588

Relates to: PR #4572

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Proposed changes

This PR mostly includes moving some methods so that they become accessible in windows environments too.

Move shell detection using gopsutil logic from shell_unix.go -> shell.go so that it can be reused in shell_windows.go as well
Remove shell_linux.go/ shell_darwin.go as they only contained a single constant that I moved to shell_unix.go
Remove getProcessEntry and related methods from shell_windows.go as we're reusing gopsutil approach that is working well on Windows

Testing

Start CRC in windows environment
Verify crc oc-env is working as expected in
- CMD
- PowerShell
- GitBash
- WSL
  - bash
  - fish
  - zsh

Contribution Checklist

I Keep It Small and Simple: The smaller the PR is, the easier it is to review and have it merged
I have performed a self-review of my code
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Which platform have you tested the code changes on?
- Linux
- Windows
- MacOS

openshift-ci · 2025-02-03T05:45:18Z

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

cfergeau · 2025-02-03T10:58:52Z

pkg/os/util.go

@@ -127,3 +127,12 @@ func RemoveFileGlob(glob string) error {
 	}
 	return nil
 }
+
+func IsPresentInList(arr []string, str string) bool {


It already exists as Contains

Thanks, I was going to use this but I noticed a unit test is failing on Windows due to these changes. This line was returning bash on Windows powershells:

crc/cmd/crc/cmd/start_test.go

Line 41 in 6d72284

userShell, err := shell.GetShell("")

On checking the process tree during unit test execution, it shows processes like this:
cmd.test.exe -> go.exe -> bash.exe

I've added another utility method in order to just match the prefix instead of matching entire string.

cfergeau · 2025-02-03T11:16:04Z

pkg/os/shell/shell.go

+		if err != nil {
+			return ""
+		}
+		if crcos.IsPresentInList(supportedShell, processName) {


This would be easier to review if it did not mix code movement with functional changes. I'd suggest having a commit where this code is moved to shell.go without any changes, and then a commit porting the Windows code to use detectShellByCheckingProcessTree with these crcos.IsPresentInList and such.

Thanks, I've split the PR into two commits with first commit containing code reorganization and second including actual change

pkg/os/shell/shell.go

cfergeau · 2025-02-04T10:25:15Z

pkg/strings/strings.go

+	})
+}
+
+func IsPresentInListSatisfying(input []string, toMatch string, matchingPredicate func(string, string) bool) bool {


While looking for naming ideas in go standard packages (usual name for such a function is ContainsFunc), I realized the slices package has both Contains and ContainsFunc, we can use these instead.

Oh, thanks a lot! I'm sorry I should've checked this.

cfergeau

There's an outdated + Add a utility method in strings for verifying a slice contains an element matching the provided predicate. in the commit log of the second commit, otherwise looks good to me!

rohanKanojia · 2025-02-04T13:32:37Z

@cfergeau : Thanks for noticing! I've updated the commit message.

I'll also create a follow up PR to make use of slice.Contains to replace the equivalent method in crcstrings.Contains

…ows refactor Related to crc-org#4588 Move shell detection related methods from shell_unix.go to shell.go so that they can be accessed from Windows code as well Signed-off-by: Rohan Kumar <rohaan@redhat.com>

+ Update `shell_windows.go` to use detectShellByCheckingProcessTree instead of relying on SHELL environment variable. + Remove hardcoded check from detectShellByCheckingProcessTree for shell types, use already present supportedShell slice. Signed-off-by: Rohan Kumar <rohaan@redhat.com>

openshift-ci · 2025-02-05T08:37:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau

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~~ [cfergeau]

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

openshift-ci · 2025-02-05T10:19:01Z

@rohanKanojia: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/security	`8398309`	link	false	`/test security`
ci/prow/e2e-crc	`8398309`	link	true	`/test e2e-crc`

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-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress label Feb 3, 2025

rohanKanojia force-pushed the pr/issue4588 branch from 8bb4473 to 9f7677b Compare February 3, 2025 06:55

cfergeau reviewed Feb 3, 2025

View reviewed changes

rohanKanojia force-pushed the pr/issue4588 branch 3 times, most recently from 1d18663 to 9e42792 Compare February 3, 2025 16:43

rohanKanojia marked this pull request as ready for review February 4, 2025 09:41

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 4, 2025

openshift-ci bot requested review from evidolob and gbraad February 4, 2025 09:42

cfergeau reviewed Feb 4, 2025

View reviewed changes

rohanKanojia force-pushed the pr/issue4588 branch from 9e42792 to 6864d0e Compare February 4, 2025 13:21

cfergeau approved these changes Feb 4, 2025

View reviewed changes

openshift-ci bot assigned cfergeau Feb 4, 2025

openshift-ci bot added lgtm approved labels Feb 4, 2025

rohanKanojia force-pushed the pr/issue4588 branch from 6864d0e to 1d5be39 Compare February 4, 2025 13:28

openshift-ci bot removed the lgtm label Feb 4, 2025

rohanKanojia mentioned this pull request Feb 4, 2025

refactor: replace custom slice Contains method with slices.Contains #4596

Merged

12 tasks

rohanKanojia added 2 commits February 5, 2025 13:52

refactor (shell) : Prerequisite code reorganization for gopsutil wind…

08ea9a0

…ows refactor Related to crc-org#4588 Move shell detection related methods from shell_unix.go to shell.go so that they can be accessed from Windows code as well Signed-off-by: Rohan Kumar <rohaan@redhat.com>

rohanKanojia force-pushed the pr/issue4588 branch from 1d5be39 to 8398309 Compare February 5, 2025 08:22

cfergeau approved these changes Feb 5, 2025

View reviewed changes

openshift-ci bot added the lgtm label Feb 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor (shell) : detect shell on windows using gopsutil too (#4588) #4591

refactor (shell) : detect shell on windows using gopsutil too (#4588) #4591

rohanKanojia commented Feb 3, 2025 •

edited

Loading

openshift-ci bot commented Feb 3, 2025

cfergeau Feb 3, 2025

rohanKanojia Feb 3, 2025 •

edited

Loading

cfergeau Feb 3, 2025

rohanKanojia Feb 3, 2025

cfergeau Feb 4, 2025

rohanKanojia Feb 4, 2025

cfergeau left a comment

rohanKanojia commented Feb 4, 2025

openshift-ci bot commented Feb 5, 2025

openshift-ci bot commented Feb 5, 2025

refactor (shell) : detect shell on windows using gopsutil too (#4588) #4591

Are you sure you want to change the base?

refactor (shell) : detect shell on windows using gopsutil too (#4588) #4591

Conversation

rohanKanojia commented Feb 3, 2025 • edited Loading

Description

Type of change

Proposed changes

Testing

Contribution Checklist

openshift-ci bot commented Feb 3, 2025

cfergeau Feb 3, 2025

Choose a reason for hiding this comment

rohanKanojia Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

cfergeau Feb 3, 2025

Choose a reason for hiding this comment

rohanKanojia Feb 3, 2025

Choose a reason for hiding this comment

cfergeau Feb 4, 2025

Choose a reason for hiding this comment

rohanKanojia Feb 4, 2025

Choose a reason for hiding this comment

cfergeau left a comment

Choose a reason for hiding this comment

rohanKanojia commented Feb 4, 2025

openshift-ci bot commented Feb 5, 2025

openshift-ci bot commented Feb 5, 2025

rohanKanojia commented Feb 3, 2025 •

edited

Loading

rohanKanojia Feb 3, 2025 •

edited

Loading