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

Fix unbound variable error in install script #3601

Merged
merged 1 commit into from
Mar 6, 2025

Conversation

brettdh
Copy link
Contributor

@brettdh brettdh commented Mar 4, 2025

If the docker-compose version was less than the minimum required version, the install script would error out like this:

❯ ./install.sh
<snip>
▶ Checking minimum requirements ...
Found Docker version 27.5.1
install/check-minimum-requirements.sh: line 17: install: unbound variable

With this patch, the script correctly identifies the out-of-date docker-compose version and bails out with a more helpful message:

❯ ./install.sh
<snip>
▶ Checking minimum requirements ...
Found Docker version 27.5.1
FAIL: Expected minimum docker compose version to be 2.132.2 but found 2.32.4-desktop.1

(Note that 2.132.2 is not a real docker-compose version; I've edited the install/_min-requirements.sh file locally for testing, to make the installed version out of date.)

Also tested that the success case still works (by removing the aforementioned local edit):

❯ ./install.sh
<snip>
▶ Checking minimum requirements ...
Found Docker version 27.5.1
Found Docker Compose version 2.32.4-desktop.1

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

Copy link
Collaborator

@aminvakil aminvakil left a comment

Choose a reason for hiding this comment

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

I think the problem relies somewhere else, why COMPOSE_VERSION should not be set and therefore inbound variable, maybe it's related to #3595 ?

@brettdh Can you please paste output of these two commands?

docker-compose version
docker compose version

@BYK
Copy link
Member

BYK commented Mar 5, 2025

Not sure why or how this happens but should be solved with #3604.

@brettdh
Copy link
Contributor Author

brettdh commented Mar 5, 2025

@aminvakil

❯ docker-compose version
Docker Compose version v2.32.4-desktop.1

❯ docker compose version
Docker Compose version v2.32.4-desktop.1

@brettdh
Copy link
Contributor Author

brettdh commented Mar 5, 2025

I figured out the root cause by adding | tee ./vergte.out to the $(vergte ...) invocation in the script and line named in the error output. Here's the content:

❯ cat vergte.out
Error in install/_lib.sh:57.
'sort --version-sort --check=quiet --reverse' exited with status 1
-> ./install.sh:main:20
--> install/check-minimum-requirements.sh:source:17
---> install/_lib.sh:vergte:57

Cleaning up...
1

Because cleanup() in install/error-handling.sh prints to stdout rather than stderr, this output gets printed as part of vergte, then the caller captures it and treats part of it as a variable named install, which is unbound, hence the error.

Changing vergte to just rely on its own exit code rather than forwarding the exit code of sort via stdout resolves this. Changing cleanup() to print to stderr would also likely resolve it, but I don't see a reason that vergte needs to use stdout when the exit code is all that matters.

@aminvakil
Copy link
Collaborator

There was a PR changing vergte function, so that it would not rely on return code, but I cannot find and don't remember why it wasn't merged, maybe I was dreaming someone had changed it ? :)

@BYK
Copy link
Member

BYK commented Mar 5, 2025

Okay, if tests etc pass I think I'm convinced that this is a better approach.

Huge props to @brettdh for the detective work and the patience to explain it to us!

@BYK BYK enabled auto-merge (squash) March 5, 2025 19:58
auto-merge was automatically disabled March 5, 2025 20:03

Head branch was pushed to by a user without write access

@brettdh
Copy link
Contributor Author

brettdh commented Mar 5, 2025

Fixed the formatting violations that pre-commit flagged.

@hubertdeng123
Copy link
Member

Other than that, this change seems reasonable

@brettdh brettdh force-pushed the fix-unbound-variable branch from 74d901f to e9ef377 Compare March 5, 2025 20:11
@brettdh
Copy link
Contributor Author

brettdh commented Mar 5, 2025

I think I'm fighting with vscode auto save behavior on the formatting issues. Sorry for the noise; will actually set up pre-commit and fix it that way soon.

@brettdh brettdh force-pushed the fix-unbound-variable branch 2 times, most recently from d587721 to 0cbc3d9 Compare March 5, 2025 21:13
@brettdh brettdh force-pushed the fix-unbound-variable branch from 0cbc3d9 to f7052fe Compare March 5, 2025 21:16
@brettdh
Copy link
Contributor Author

brettdh commented Mar 5, 2025

Ok it should be good now, including formatting and conflict resolution with recent changes on master.

@BYK BYK merged commit 979f219 into getsentry:master Mar 6, 2025
5 checks passed
@brettdh brettdh deleted the fix-unbound-variable branch March 7, 2025 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants