tfutils / tfenv

Terraform version manager
MIT License
4.42k stars 446 forks source link

Fix darwin arch selection logic #388

Closed alex-shafer-1002 closed 6 months ago

alex-shafer-1002 commented 1 year ago

This corrects the regex to choose amd64 for versions < 1.0.2

Tested as follows:

  1. Created a script to test the logic in isolation:
#!/usr/bin/env bash

while read version; do
    if [[ "${version}" =~ ^0\. || "${version}" =~ ^1\.0\.[01]$ ]]; then
      echo "amd64  $version"
    else
      echo "ARM64  $version"
    fi;
done
  1. Piped tfenv list-remote to this script to test all current versions:
tfenv list-remote | ~/version-logic.sh
  1. Verified that the output switched from amd64 to arm64 at the expected version (truncated for brevity):
ARM64  1.0.4
ARM64  1.0.3
ARM64  1.0.2
amd64  1.0.1
amd64  1.0.0
amd64  0.15.5
  1. Temporarily patched my local installed libexec/tfenv-install with no TFENV_ARCH set in my environment and tested some relevant cases (note the arch in the download URL):
% tfenv install 1.0.1
Installing Terraform v1.0.1
Downloading release tarball from https://releases.hashicorp.com/terraform/1.0.1/terraform_1.0.1_darwin_amd64.zip
...
Installation of terraform v1.0.1 successful. To make this your default version, run 'tfenv use 1.0.1'

% tfenv install 1.0.2
Installing Terraform v1.0.2
Downloading release tarball from https://releases.hashicorp.com/terraform/1.0.2/terraform_1.0.2_darwin_arm64.zip
...
Installation of terraform v1.0.2 successful. To make this your default version, run 'tfenv use 1.0.2'

% tfenv install 1.0.10
Installing Terraform v1.0.10
Downloading release tarball from https://releases.hashicorp.com/terraform/1.0.10/terraform_1.0.10_darwin_arm64.zip
...
Installation of terraform v1.0.10 successful. To make this your default version, run 'tfenv use 1.0.10'

% tfenv install 1.0.11
Installing Terraform v1.0.11
Downloading release tarball from https://releases.hashicorp.com/terraform/1.0.11/terraform_1.0.11_darwin_arm64.zip
...
Installation of terraform v1.0.11 successful. To make this your default version, run 'tfenv use 1.0.11'
denizgenc commented 1 year ago

Your script produces the exact same output when run against tfutils:master. There isn't anything wrong with the line you're changing; you're probably comparing against the latest released binary on brew, which is based on the 3.0.0 label, where tfenv-install looked like this.

(sorry if I'm being a bit defensive, I wrote the line that this PR replaces šŸ˜…)

alex-shafer-1002 commented 1 year ago

Got it, yes this was against the current brew version, we just really need a new deploy I guess.

bryanhiestand commented 1 year ago

Your script produces the exact same output when run against tfutils:master. There isn't anything wrong with the line you're changing; you're probably comparing against the latest released binary on brew, which is based on the 3.0.0 label, where tfenv-install looked like this.

(sorry if I'm being a bit defensive, I wrote the line that this PR replaces šŸ˜…)

I'm having trouble following you or understanding how these two lines could produce the same output. Can you point me at the code you're talking about if it's not this? https://github.com/tfutils/tfenv/blob/1ccfddb22005b34eacaf06a9c33f58f14e816ec9/libexec/tfenv-install#L127

I just opened #403 to try to fix the bugs in these regexes, but it looks like this PR fixes the behavior as well.

Current master (but not release 3.0.0) fixes the issue where arm64 is chosen instead of amd64 for versions < 1.0.2. But with the code I linked (and this PR changes), some versions > 1.0.2 use amd64 instead of arm64.

denizgenc commented 1 year ago

You're definitely right, I didn't use the start of line anchors and should have been stricter about what I was matching with the regexes. Apologies to Alex

Zordrak commented 6 months ago

Superseded by #403