[SM-1130] - add install scripts #645

tangowithfoxtrot · 2024-03-01T15:26:37Z

Type of change

Objective

Allow easy installation of bws on most systems with:

curl <script-url> | sh on Linux and macOS
or
iwr <script-url> | iex on Windows

Code changes

crates/bws/scripts/install.(sh|ps1): added install scripts that will download bws from GitHub Releases, validate the checksums, and install it. I am unsure if this is the best place to house these scripts and would welcome suggestions for other places these might belong.
.github/workflows/version-bump.yml: added version bumps for the install scripts

Before you submit

Please add unit tests where it makes sense to do so

bitwarden-bot · 2024-03-01T15:33:02Z

Checkmarx One – Scan Summary & Details – b77dac5c-a984-4807-8833-fe7a82afa5f9

No New Or Fixed Issues Found

codecov · 2024-03-01T15:36:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.86%. Comparing base (00b2120) to head (3ff5cde).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #645   +/-   ##
=======================================
  Coverage   59.86%   59.86%           
=======================================
  Files         189      189           
  Lines       12457    12457           
=======================================
  Hits         7457     7457           
  Misses       5000     5000

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

coltonhurst · 2024-06-07T20:13:44Z

Not a bad location, but I agree we should think about where we want this. (If we do keep it in the bws crate I think it should definitely be in some type of subdirectory rather than the crate root.)

Some location ideas:

Separate directory, like crates/bws/scripts/<here>
GitHub Gist
Somewhere else?

Great idea 🎉

…thub.com/bitwarden/sdk into sm/SM-1130/host-a-generic-install-script

tangowithfoxtrot · 2024-06-11T20:49:14Z

@coltonhurst, yeah, I'm beginning to think that the first option you provided is probably the way to go to avoid cluttering things too much. I don't think a GitHub gist will do though because we'll need to be able to automatically bump the version numbers in the install scripts.

I'll wait to see if any other ideas come up before moving them there, just to give other people the chance to chime-in.

tangowithfoxtrot · 2024-06-11T20:49:55Z

.github/workflows/version-bump.yml

+          # bump the version in install.sh
+          sed -i 's/BWS_VERSION="${BWS_VERSION:-[0-9]\+\.[0-9]\+\.[0-9]\+}"/BWS_VERSION="${BWS_VERSION:-${{ inputs.version_number }}}"/' ./crates/bws/install.sh
+          # bump the version in install.ps1
+          sed -i 's/{ \$env:bwsVersion } else { "[0-9]\+\.[0-9]\+\.[0-9]\+" }/\{ \$env:bwsVersion } else { "${{ inputs.version_number }}" }/' ./crates/bws/install.ps1


These sed expressions worked when I tested them locally, but there doesn't appear to be a way to test this in the workflow, so I would appreciate some scrutiny here.

Updated with simpler regex matches in 2dba059.

we were previously matching the first checksum for the OS due to a lazy architecture match condition

coltonhurst

I know we were waiting on other reviews, but some additional brief thoughts. I can do a deeper dive once it's fully ready again.

Note: We might want to consider adding a way to uninstall as well, and make sure nothing is left on the user's system.

Note 2: Is there a ticket for this? If not we will want one and definitely will want QA to review these

coltonhurst · 2024-06-13T18:55:38Z

crates/bws/install.sh

+  elif [ "$(uname -s)" = "Darwin" ]; then
+    PLATFORM="apple-darwin"
+  else
+    error "Unsupported platform: $(uname -s)"


We could potentially point people to the GitHub SDK repository if they want to create an issue for us to look into supporting their platform.

crates/bws/install.sh

coltonhurst · 2024-06-13T19:16:32Z

crates/bws/install.sh

+  expected_checksum="$(grep "bws-${ARCH}-${PLATFORM}-${BWS_VERSION}.zip" "$checksum_file" | awk '{print $1}')"
+  actual_checksum="$(sha256sum "$tmp_dir/bws.zip" | awk '{print $1}')"


Doesn't this get the expected and actual checksum from the same downloaded zip?

This greps for the string "bws-${ARCH}-${PLATFORM}-${BWS_VERSION}.zip" in the checksum file and uses awk to return just the checksum value.

Like this:

bash-5.2$ grep bws-aarch64-apple-darwin-0.5.0.zip ~/Downloads/bws-sha256-checksums-0.5.0.txt aeac01edbd7cdfb8e75e9143d13d69221e6e0bff0fd9f8de69b85c3108a4e986 bws-aarch64-apple-darwin-0.5.0.zip bash-5.2$ grep bws-aarch64-apple-darwin-0.5.0.zip ~/Downloads/bws-sha256-checksums-0.5.0.txt | awk '{print $1}' aeac01edbd7cdfb8e75e9143d13d69221e6e0bff0fd9f8de69b85c3108a4e986

mzieniukbw

Nice scripts!
I have concern with the checksums on powershell though - did you had a chance to test it out on Windows? :)

mzieniukbw · 2024-06-17T09:02:27Z

.github/workflows/version-bump.yml

+          # bump the version in install.sh
+          sed -i 's/BWS_VERSION="${BWS_VERSION:-[0-9]\+\.[0-9]\+\.[0-9]\+}"/BWS_VERSION="${BWS_VERSION:-${{ inputs.version_number }}}"/' ./crates/bws/scripts/install.sh
+          # bump the version in install.ps1
+          sed -i 's/{ \$env:bwsVersion } else { "[0-9]\+\.[0-9]\+\.[0-9]\+" }/\{ \$env:bwsVersion } else { "${{ inputs.version_number }}" }/' ./crates/bws/scripts/install.ps1


i wonder if it wouldn't be just easier to define something likeDEFAULT_BWS_VERSION for both scripts.
Then in the install.sh this would look like this:

DEFAULT_BWS_VERSION=0.5.0 BWS_VERSION=${BWS_VERSION:-$DEFAULT_BWS_VERSION}

then from regex perspective it's much easier and consistent for both scripts

Great idea!

Fixed in 2dba059

mzieniukbw · 2024-06-17T09:57:43Z

crates/bws/scripts/install.ps1

+  $expectedChecksum = (Get-Content $checksumFile | Where-Object { $_ -match "bws-$arch-pc-windows-msvc-$bwsVersion.zip" }).Split(" ")[0]
+  $actualChecksum = (Get-FileHash -Algorithm SHA256 -Path $zipPath).Hash
+
+  if ($actualChecksum -ne $expectedChecksum) {


i think the Hash from Get-FileHash is uppercase, while the GH checksum file is lowercase, so they would not match.

From what I can tell, PowerShell is case-insensitive, where possible. I found this, which says that this is also the case for comparison operators, so the comparison should return true, despite being in different cases.

Regarding testing on Windows: I have tested it sparingly in a VM a few times throughout updating the script using iwr <url-for-raw-file> | iex, and it seems to be working.

I haven't been able to test it much on x86_64 though, as the only Windows machine in my house is a VM on my M2 Mac.

Windows as always have to be unique. Indeed it seems that string comparison is case insensitive, which is mind blowing to me 🤯
Thank you for the insights into testing, i wondered how do you test out Windows on Mac. 👍

tangowithfoxtrot · 2024-06-17T17:33:35Z

@coltonhurst, I added an uninstallation arg in 24e0e98.

It's pretty easy to invoke in Linux/macOS with:

curl https://raw.githubusercontent.com/bitwarden/sdk/2dba059c1ee17366debb9535cff34fda767c3ff9/crates/bws/scripts/install.sh | sh -s -- --uninstall

However, I can't figure out a straightforward way to pass the -Uninstall arg through Invoke-Expression in PowerShell, so the best way to perform an uninstallation on Windows will require downloading the install script locally and passing the arg directly, like this:

.\install.ps1 -Uninstall

If you have greater PowerShell finesse than me, you can probably pass the -Uninstall arg through the

iex https://raw.githubusercontent.com/bitwarden/sdk/2dba059c1ee17366debb9535cff34fda767c3ff9/crates/bws/scripts/install.ps1 | iex # not sure how to pass args to the script through this???

one-liner, but I haven't been successful in doing so.

mzieniukbw

Approved, but would like to know other's feedback @coltonhurst on whether uninstalling should also remove .bws config directory - i am personally fine with it.

tangowithfoxtrot · 2024-06-20T21:20:23Z

@coltonhurst, Added mention of the install scripts in the README in 035ca93

coltonhurst · 2024-06-21T15:03:31Z

Approved, but would like to know other's feedback @coltonhurst on whether uninstalling should also remove .bws config directory - i am personally fine with it.

Yeah I think we should make sure everything is cleared so there is no trace on the users system.

add install scripts

b39cc93

tangowithfoxtrot added 10 commits June 5, 2024 07:59

Merge branch 'main' into sm/SM-1130/host-a-generic-install-script

9d72a15

fix arm arch and fmt error

25464c8

allow overriding bws version; use Bitwarden for path name

51f1fc8

update to 0.5.0

c798283

fix erroneous newline

276beef

add checksum validation

1e4a441

Merge branch 'main' into sm/SM-1130/host-a-generic-install-script

3160a61

rm todo; grammar

83f53d3

fix macOS arch check; quote vars

f16cf6d

fix macOS arm arch; quote vars

34e840e

tangowithfoxtrot added 3 commits June 11, 2024 13:14

Merge branch 'sm/SM-1130/host-a-generic-install-script' of https://gi…

a3ff347

…thub.com/bitwarden/sdk into sm/SM-1130/host-a-generic-install-script

use PS-friendly names

c236af2

add version bumps for install scripts

4aa9bc2

tangowithfoxtrot commented Jun 11, 2024

View reviewed changes

Merge branch 'main' into sm/SM-1130/host-a-generic-install-script

c0e67ba

tangowithfoxtrot marked this pull request as ready for review June 11, 2024 21:10

tangowithfoxtrot requested a review from a team as a code owner June 11, 2024 21:10

fix: match correct checksum for arch

b6e0b3f

we were previously matching the first checksum for the OS due to a lazy architecture match condition

tangowithfoxtrot requested a review from a team June 12, 2024 13:08

coltonhurst requested changes Jun 13, 2024

View reviewed changes

tangowithfoxtrot added 2 commits June 13, 2024 14:21

fix: checksum validation for macs without coreutils

0dd2fda

move to scripts dir

0c3b568

mzieniukbw requested changes Jun 17, 2024

View reviewed changes

tangowithfoxtrot added 2 commits June 17, 2024 10:10

add uninstallation arg

24e0e98

use easier regex matching

2dba059

mzieniukbw previously approved these changes Jun 17, 2024

View reviewed changes

tangowithfoxtrot changed the title ~~add install scripts~~ [SM-1130] - add install scripts Jun 20, 2024

mention install script in docs

035ca93

tangowithfoxtrot dismissed mzieniukbw’s stale review via 035ca93 June 20, 2024 21:19

tangowithfoxtrot requested a review from coltonhurst June 20, 2024 21:22

Merge branch 'main' into sm/SM-1130/host-a-generic-install-script

3ff5cde

mzieniukbw approved these changes Jun 21, 2024

View reviewed changes

michalchecinski approved these changes Jun 21, 2024

View reviewed changes

coltonhurst approved these changes Jun 21, 2024

View reviewed changes

coltonhurst merged commit 56547d7 into main Jun 28, 2024
105 checks passed

coltonhurst deleted the sm/SM-1130/host-a-generic-install-script branch June 28, 2024 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SM-1130] - add install scripts #645

[SM-1130] - add install scripts #645

tangowithfoxtrot commented Mar 1, 2024 •

edited

Loading

bitwarden-bot commented Mar 1, 2024 •

edited by github-actions bot

Loading

codecov bot commented Mar 1, 2024 •

edited

Loading

coltonhurst commented Jun 7, 2024 •

edited

Loading

tangowithfoxtrot commented Jun 11, 2024

tangowithfoxtrot Jun 11, 2024

tangowithfoxtrot Jun 17, 2024

coltonhurst left a comment •

edited

Loading

coltonhurst Jun 13, 2024

coltonhurst Jun 13, 2024

tangowithfoxtrot Jun 13, 2024

mzieniukbw left a comment

mzieniukbw Jun 17, 2024 •

edited

Loading

tangowithfoxtrot Jun 17, 2024

tangowithfoxtrot Jun 17, 2024

mzieniukbw Jun 17, 2024

tangowithfoxtrot Jun 17, 2024

tangowithfoxtrot Jun 17, 2024

mzieniukbw Jun 17, 2024

tangowithfoxtrot commented Jun 17, 2024

mzieniukbw left a comment

tangowithfoxtrot commented Jun 20, 2024

coltonhurst commented Jun 21, 2024

		expected_checksum="$(grep "bws-${ARCH}-${PLATFORM}-${BWS_VERSION}.zip" "$checksum_file" \| awk '{print $1}')"
		actual_checksum="$(sha256sum "$tmp_dir/bws.zip" \| awk '{print $1}')"

[SM-1130] - add install scripts #645

[SM-1130] - add install scripts #645

Conversation

tangowithfoxtrot commented Mar 1, 2024 • edited Loading

Type of change

Objective

Code changes

Before you submit

bitwarden-bot commented Mar 1, 2024 • edited by github-actions bot Loading

No New Or Fixed Issues Found

codecov bot commented Mar 1, 2024 • edited Loading

Codecov Report

coltonhurst commented Jun 7, 2024 • edited Loading

tangowithfoxtrot commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coltonhurst left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzieniukbw left a comment

Choose a reason for hiding this comment

mzieniukbw Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangowithfoxtrot commented Jun 17, 2024

mzieniukbw left a comment

Choose a reason for hiding this comment

tangowithfoxtrot commented Jun 20, 2024

coltonhurst commented Jun 21, 2024

tangowithfoxtrot commented Mar 1, 2024 •

edited

Loading

bitwarden-bot commented Mar 1, 2024 •

edited by github-actions bot

Loading

codecov bot commented Mar 1, 2024 •

edited

Loading

coltonhurst commented Jun 7, 2024 •

edited

Loading

coltonhurst left a comment •

edited

Loading

mzieniukbw Jun 17, 2024 •

edited

Loading