Conversation
The linux branch of getUpdateInstructions never inspected execPath outside the npm check, falling through to getLinuxPackageManager(), which only probes which package managers exist on the machine. A nix install on Debian or Ubuntu was told to run 'apt-get install infisical' for a binary in the immutable nix store. Homebrew-on-Linux hit the same gap, since /home/linuxbrew/.linuxbrew/ does not match the /homebrew/ substring the darwin branch checks. Mirrors the existing darwin behaviour: nix returns no instruction rather than a wrong one. Adds unit cases and integration steps to the integration-linux job. Partially addresses Infisical/infisical#226
Review feedback: a bare "linuxbrew" substring also classified unrelated paths such as /opt/linuxbrew-tools as Homebrew installs. Uses "linuxbrew/" rather than a fully bounded "/linuxbrew/" so both real layouts still match: the multi-user default (/home/linuxbrew/.linuxbrew) and the single-user prefix (~/.linuxbrew), where the component is preceded by a dot rather than a slash. Adds a single-user case and a negative case for /opt/linuxbrew-tools, plus a notExpected field on the test table to express the negative assertion.
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The trailing-slash match still classified an unrelated directory such as /home/user/linuxbrew/bin as a Homebrew install and printed a brew upgrade command that cannot update that binary. Both documented Homebrew-on-Linux layouts put the prefix at .linuxbrew, so match that instead. Adds a regression test for the plain linuxbrew path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-raising #344, which GitHub closed automatically when I deleted my fork. Code and tests are unchanged from the original, including the follow-up commit that bounds the Linuxbrew match to
linuxbrew/. Sorry for the new number.Description 📣
On Linux,
getUpdateInstructionsnever inspects the executable path outside the npm check. It falls through togetLinuxPackageManager(), which only probes which package managers happen to exist on the machine, so a Nix install on Debian or Ubuntu is told to runapt-get install infisicalfor a binary living in the immutable Nix store.Homebrew-on-Linux hits the same gap, since
/home/linuxbrew/.linuxbrew/does not match the/homebrew/substring the darwin branch checks for.This mirrors the existing darwin handling: Nix returns no instruction rather than a wrong one, matching the
darwin nix returns emptycase already in the tests. Linuxbrew returns the brew command.Fixes the Linux side of #226. The macOS path in that report already returns no instruction for Nix installs, via the existing
darwin nix returns emptyhandling.Type ✨
Tests 🛠️
Added two cases to
TestGetUpdateInstructions, and two steps to theintegration-linuxjob following the existing/tmp/homebrewand npm patterns.Worth noting for review: the
linux nix returns emptyunit case only asserts anything on a machine that actually has a package manager installed. On a dev Mac it passes with or without this change, becausegetLinuxPackageManager()returns empty there anyway. On the ubuntu runner, where apt-get is present, it fails without the fix. That is why the integration step matters here, and I left a comment above it explaining the same thing.go test ./packages/util/ -run TestGetUpdateInstructions -vVerified the new tests are meaningful by stashing the change in
check-for-update.goand re-running:linux_linuxbrewfails as expected.