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

feat: invoke dbeaver inside wsl2, fixes #5375 #5916

Merged
merged 3 commits into from Mar 5, 2024
Merged

Conversation

Brupes
Copy link
Contributor

@Brupes Brupes commented Mar 1, 2024

DBeaver support inside wsl2

The Issue

We are not able to call ddev dbeaver inside wsl2

How This PR Solves The Issue

Command dbeaver was modified to support dbeaver community installed to everyone

Manual Testing Instructions

Install DBeaver Community edition in windows and call ddev dbeaver from wsl2.
It should open dbeaver and add the connection options to current project

Related Issue Link(s)

@Brupes Brupes requested a review from a team as a code owner March 1, 2024 02:20
Copy link

github-actions bot commented Mar 1, 2024

@rfay
Copy link
Member

rfay commented Mar 1, 2024

Artifacts are ready for testing, #5916 (comment)

@Brupes
Copy link
Contributor Author

Brupes commented Mar 1, 2024

everything working on my side 😊
image

@rfay
Copy link
Member

rfay commented Mar 1, 2024

Yay @Brupes - so you installed the built DDEV version and it worked OK? Your test isn't valid if you had an altered dbeaver in your ~/.ddev/commands/host, so please make sure you delete that when testing. It should be recreated on ddev start with the one from the artifacts here.

@Brupes
Copy link
Contributor Author

Brupes commented Mar 1, 2024

Just checked it now. everything looks ok.
image

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

@Brupes
Copy link
Contributor Author

Brupes commented Mar 3, 2024

For commands
image

For database management
image

does this look ok? i am not so sure about commands documentation :/

@rfay
Copy link
Member

rfay commented Mar 3, 2024

It looks like you're on the right track to me, thanks. Push it up!

@Brupes Brupes requested a review from a team as a code owner March 3, 2024 19:55
@rfay
Copy link
Member

rfay commented Mar 3, 2024

Unfortunately this doesn't work if Dbeaver is installed with the defaults - it ends up in C:\Users\randy\AppData\Local\DBeaver\dbeaver.exe for me.

It does work if I install with "install for all users". I'm not sure how we can explain this to people, or if we can add something that adds the new path.

But it does work fine on WSL2 if installed for all users.

Of course, folks on WSL2 may also want to use the DBeaver linux version, but we'll leave that for another time.

@rfay
Copy link
Member

rfay commented Mar 3, 2024

Still works fine on macOS

@Brupes
Copy link
Contributor Author

Brupes commented Mar 3, 2024

hmm... investigating the issue with user profile path which is a dynamic one, i discovered it is actually possible to get it from wsl2

cmdWSL2Path=$(wslpath -au 'C:\Windows\System32\cmd.exe')
userProfileWindowsPath=$("$cmdWSL2Path" /c 'echo %USERPROFILE%')
userProfileWSL2Path=$(wslpath -au "$userProfileWindowsPath")

the var userProfileWSL2Path will contain the path to current windows profile path from wsl2

the problem is to add it to HostBinaryExists :/

@Brupes
Copy link
Contributor Author

Brupes commented Mar 3, 2024

i got a better way of doing it now. i wanted to remove an output that would get us trouble later because of UNC paths and i ended with this code now

userProfileWindowsPath=$(pushd /mnt/c > /dev/null; cmd.exe /c 'echo %USERPROFILE%'; popd > /dev/null)
userProfileWSL2Path=$(wslpath -au "$userProfileWindowsPath")

@Brupes
Copy link
Contributor Author

Brupes commented Mar 3, 2024

This is working. The problem now is how i can add this path to HostBinaryExists...

case $OSTYPE in
  "linux-gnu")
    # Check for different binaries. Launch the first one found.
    BINARIES=(
      /usr/bin/dbeaver{,-ce,-le,-ue,-ee}
      /var/lib/flatpak/exports/bin/io.dbeaver.DBeaverCommunity
      /snap/bin/dbeaver-ce
      '/mnt/c/Program Files/dbeaver/dbeaver.exe'
    )
    for binary in "${BINARIES[@]}"; do
      if [ -x "$binary" ]; then
        echo "Launching $binary"
        "$binary" -con "$CONNECTION" &> /dev/null & disown
        exit 0
      fi
    done

    # Check for different binaries inside user profile wsl2. Launch the first one found.
    if [ -x "/mnt/c/Windows/System32/cmd.exe" ]; then
      BINARIES=(
        AppData/Local/dbeaver/dbeaver.exe
      )
      prefix="$(wslpath -au "$(pushd /mnt/c > /dev/null; cmd.exe /c 'echo %USERPROFILE%' | tr -d '\r\n'; popd > /dev/null)")"
      for binary in "${BINARIES[@]}"; do
        if [ -x "$prefix/$binary" ]; then
          echo "Launching $prefix/$binary"
          "$prefix/$binary" -con "$CONNECTION" &> /dev/null & disown
          exit 0
        fi
      done
    fi
    ;;
  "darwin"*)
    open -a dbeaver.app --args -con "$CONNECTION" &
    echo "Attempted to launch DBeaver.app"
    ;;
esac

image

@rfay
Copy link
Member

rfay commented Mar 4, 2024

I think it's probably OK to go with the global installation. Add something to https://ddev.readthedocs.io/en/latest/users/usage/commands/#dbeaver that explains that it has to be the global installation.

@Brupes
Copy link
Contributor Author

Brupes commented Mar 5, 2024

sorry the delay. i was working in other stuff 😅
here you have. just added "to all users". think it's good. the path is already there to reinforce that soo... 😊

Open DBeaver with the current project’s database (global shell host container command). This command is only available if DBeaver.app is installed as /Applications/DBeaver.app for macOS, if dbeaver.exe is installed to all users as C:/Program Files/dbeaver/dbeaver.exe for WSL2, and if dbeaver (or another binary like dbeaver-ce) available inside /usr/bin for Linux (Flatpak and snap support included).

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me! It was testing fine before, I assume nothing has changed that makes me do manual test again.

@Brupes
Copy link
Contributor Author

Brupes commented Mar 5, 2024

Thanks, looks good to me! It was testing fine before, I assume nothing has changed that makes me do manual test again.

Same. i just updated the documentation files. it should have the same behaviour as before

@rfay rfay merged commit 3a1ec30 into ddev:master Mar 5, 2024
18 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.

None yet

2 participants