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

Shell: Use ps command which works for linux and mac #4538

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

praveenkumar
Copy link
Member

As part of 74d1f1b ps command has --sort=-pid which doesn't work with macOS by default so in this PR --sort=-pid option is removed and reverse sorting against pid is implemented in inspectProcessOutputForRecentlyUsedShell. This way this code works on mac/linux same way.

fixes: #4537

  • Which platform have you tested the code changes on?
    • Linux
    • Windows
    • MacOS

@openshift-ci openshift-ci bot requested review from anjannath and evidolob January 2, 2025 16:04
@rohanKanojia
Copy link
Contributor

@praveenkumar : Shall we also update the windows version?

return detectShellByInvokingCommand("bash", "wsl", []string{"-e", "bash", "-c", "ps -ao pid=,comm= --sort=-pid"})

@praveenkumar
Copy link
Member Author

@praveenkumar : Shall we also update the windows version?

Done

release-info.json-e Outdated Show resolved Hide resolved
As part of 74d1f1b ps command
has `--sort=-pid` which doesn't work with macOS by default so in this
PR `--sort=-pid` option is removed and reverse sorting against pid is
implemented in `inspectProcessOutputForRecentlyUsedShell`. This way
this code works on mac/linux same way.

fixes: crc-org#4537
@rohanKanojia
Copy link
Contributor

Thanks for the quick fix!

I've tested this change on the following environments and it seems to be working as expected:

  • ✔️ Windows PowerShell
    $Env:PATH = "C:\Users\rokum\.crc\bin\oc;$Env:PATH"
    # Run this command to configure your shell:
    # & crc oc-env | Invoke-Expression
  • ✔️ Windows Command Prompt
    SET PATH=C:\Users\rokum\.crc\bin\oc;%PATH%
    REM Run this command to configure your shell:
    REM @FOR /f "tokens=*" %i IN ('crc oc-env') DO @call %i
  • ✔️ Windows Git Bash
    export PATH="/C/Users/rokum/.crc/bin/oc:$PATH"
    # Run this command to configure your shell:
    # eval $(crc oc-env)
  • ✔️ Windows bash shell on WSL
    rohan@rokumar-lenovo:~$ /mnt/c/Users/rokum/go/bin/crc.exe oc-env
    export PATH="/mnt/c/Users/rokum/.crc/bin/oc:$PATH"
    # Run this command to configure your shell:
    # eval $(crc oc-env)
  • ✔️ Windows zsh shell on WSL
    rohan@rokumar-lenovo ~ % /mnt/c/Users/rokum/go/bin/crc.exe oc-env
    export PATH="/mnt/c/Users/rokum/.crc/bin/oc:$PATH"
    # Run this command to configure your shell:
    # eval $(crc oc-env)
  • ✔️ Windows fish shell on WSL
     rohan@rokumar-lenovo ~> /mnt/c/Users/rokum/go/bin/crc.exe oc-env
     contains /mnt/c/Users/rokum/.crc/bin/oc $fish_user_paths; or set -U fish_user_paths /mnt/c/Users/rokum/.crc/bin/oc 
  • ✔️ Linux bash shell
    crc : $ crcdev oc-env
    export PATH="/home/rohan/.crc/bin/oc:$PATH"
    # Run this command to configure your shell:
    # eval $(crc oc-env)
  • ✔️ Linux zsh shell
     rohan@rohan-20y4s1qe0p ~/go/src/github.com/crc-org/crc
     % crcdev oc-env
     export PATH="/home/rohan/.crc/bin/oc:$PATH"
     # Run this command to configure your shell:
     # eval $(crc oc-env)
  • ✔️ Linux fish shell
     rohan@rohan-20y4s1qe0p ~/g/s/g/c/crc (issue_4537)> crcdev oc-env
     contains /home/rohan/.crc/bin/oc $fish_user_paths; or set -U fish_user_paths /home/rohan/.crc/bin/oc $fish_user_paths
     # Run this command to configure your shell:
     # eval (crc oc-env)

Copy link

openshift-ci bot commented Jan 2, 2025

@praveenkumar: 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/integration-crc 0400330 link true /test integration-crc
ci/prow/e2e-crc 0400330 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.

@praveenkumar
Copy link
Member Author

@lilyLuLiu can you run nighty test using that pr and see if it works for all platform.

@lilyLuLiu
Copy link
Contributor

@praveenkumar Tests on this pr passed on macOS-arm64, but failed on macOS-x86

@lilyLuLiu
Copy link
Contributor

Manually test on macOS-x86 passed.

@praveenkumar
Copy link
Member Author

@praveenkumar Tests on this pr passed on macOS-arm64, but failed on macOS-x86

We tried to test manually on x86 ( Thanks @lilyLuLiu for sharing the details) and figure out it is something related to testing side (test used older version of binary).

macmini-crcqe-2:~ crcqe $ crc version
CRC version: 25.01.03+040033
OpenShift version: 4.17.7
MicroShift version: 4.17.7

macmini-crcqe-2:~ crcqe$ crc oc-env --log-level debug
DEBU CRC version: 25.01.03+040033                 
DEBU OpenShift version: 4.17.7                    
DEBU MicroShift version: 4.17.7                   
DEBU Running 'crc oc-env'                         
DEBU Running 'ps -o pid=,comm='                   
DEBU Detected shell: -bash                        
export PATH="/Users/crcqe/.crc/bin/oc:$PATH"
# Run this command to configure your shell:
# eval $(crc oc-env)

macmini-crcqe-2:~ crcqe$ eval $(crc oc-env)

Copy link

openshift-ci bot commented Jan 3, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gbraad, rohanKanojia

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:

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

@openshift-ci openshift-ci bot added the approved label Jan 3, 2025
@praveenkumar praveenkumar merged commit a697b9e into crc-org:main Jan 6, 2025
29 of 36 checks passed
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.

command 'eval $(crc oc-env)' fail on mac
4 participants