FIX Support default tofu version #41

orbitz · 2024-01-20T07:35:19Z

Support an environment variable for the default version of OpenTofu if no other version information is found.

Being an environment variable gives more flexibility than the existing ``${TOFUENV_CONFIG_DIR}/version` solution, in that you can easily set the environment variable for the process you are running.

This resolves #34

Nmishin · 2024-01-20T18:21:20Z

Hi @orbitz thank you for MR!
Could you please check the tests, test_uninstall failed for some reason ...

kvendingoldo · 2024-01-20T22:33:38Z

LGTM; As @Nmishin said, it would be good idea to fix tests if you have capacity for that. Big thanks for contribution

orbitz · 2024-01-21T06:28:33Z

I'm not quite sure how to read the output here, I don't see a failure. Any ideas how to track it down?

orbitz · 2024-01-21T06:38:23Z

I added some explicit logging statements to the tests which will hopefully narrow it down, but need the tests to run here.

anastasiiakozlova245 · 2024-01-21T17:28:04Z

lib/tofuenv-version-name.sh

+            && log 'debug' "TOFUENV_VERSION specified in TOFUENV_VERSION_FILE: ${TOFUENV_VERSION}";
+        TOFUENV_VERSION_SOURCE="${TOFUENV_VERSION_FILE}";
+    else
+        TOFUENV_VERSION="${TOFUENV_TOFU_DEFAULT_VERSION:-"latest"}"


Hi @orbitz, thanks for your contribution!
The default value should be empty if no version is set in TOFUENV_TOFU_DEFAULT_VERSION variable. Otherwise, it will always trigger the installation of the latest version when tofuenv version-name command is triggered (which is not supposed to trigger installation). This is what causes the tests' failure.

Isn't this what the documentation for tofuenv install says should happen, though?

If no parameter is passed, the version to use is resolved automatically via TOFUENV_TOFU_VERSION environment variable or .opentofu-version files, in that order of precedence, i.e. TOFUENV_TOFU_VERSION, then .opentofu-version. The default is latest if none are found.

Yes, but tofuenv install already works this way.
It calls tofuenv-resolve-version first - https://github.com/tofuutils/tofuenv/blob/main/libexec/tofuenv-install#L67, and then installs latest if no other version has been specified - https://github.com/tofuutils/tofuenv/blob/main/libexec/tofuenv-resolve-version#L97

Actually, tofuenv install command doesn't use tofuenv-version-name.sh script at all (tofuenv version-name command just checks and returns opentofu version currently used in a current directory, so it should not trigger any installation). So to add the support of a new TOFUENV_TOFU_DEFAULT_VERSION variable during installation process, you'll need to add a correspoding change to libexec/tofuenv-install script as well.

Just noticed that tofuenv version-name command is not described in the documentation, we'll add it there. But the output of tofuenv help returns the following description:

version-name Print current version of OpenTofu

So this command should not trigger any installation and should just show the current OpenTofu version. If it's set to latest by default (when no version is specified in TOFUENV_TOFU_VERSION environment variable, in .opentofu-version file, in version file, or in TOFUENV_TOFU_DEFAULT_VERSION variable), it's illogical, when a user doesn't have any OpenTofu installed. Moreover, setting it to latest by default if no version is specified anywhere will trigger OpenTofu installation, when a user just wants to check if they have any versions installed, but not to install anything.
So if no OpenTofu version is specified in the list of sources to check, it should remain empty when calling tofuenv version-name.

However, you're right that tofuenv install command should install latest version if no version is specified - but it's set in a different script, to which I attached the link above. That would be great if you could add support for the TOFUENV_TOFU_DEFAULT_VERSION there 🙂

Thank you for the explanation. After looking over the files you mentioned I see my mistake.

At the same time, I wonder if perhaps the behaviour of this change is correct? It depends on what one expects tofuenv install to do. In my case, I expected it to install whatever version the current directory specified. But if the actual functionality is to install the specified version, or latest, then it is correct (although my README update is wrong).

As it is now, the current behavior does what I want in that tofu ... will install the version specified in the directory or fallback to an environment variable. However, as you mention, it does not for tofu install.

What would you like the behaviour of these to operations to be?

Currently, as you mentioned, tofuenv install command first checks if any opentofu version is set in TOFUENV_TOFU_VERSION environment variable, and then in .opentofu-version file, and if no versions are found - uses latest.
But as you mentioned in README, it would be great to support default version setting. In this case, tofuenv install command without any version passed will check the following sources:

TOFUENV_TOFU_VERSION environment variable

.opentofu-version files

TOFUENV_TOFU_DEFAULT_VERSION

The default of latest.

So we will still have latest version if no version was specified, but also there will be an ability to set a default one using TOFUENV_TOFU_DEFAULT_VERSION environment variable in, for ex., some CI/CD pipeline

Great! Just confirming the desired behaviour. I'll finish this change it ASAP

Nmishin · 2024-01-27T14:35:34Z

@orbitz could you please also include changelog file to your MR, for example as here: https://github.com/tofuutils/tofuenv/pull/50/files

this will allow us to have a beautiful history in CHANGELOG.md

orbitz · 2024-01-28T09:27:25Z

This is ready for review again.

orbitz · 2024-01-29T13:19:57Z

Looks like this is failing due to uninstall again. After some debugging, the cause is because of my change on any tofu operation, we install according to the ruleset, so the tofu command never fails because no version is installed (unless auto-install is turned off).

What do you think are the appropriate semantics here?

orbitz · 2024-01-29T13:29:53Z

test/test_uninstall.sh

@@ -56,7 +56,7 @@ function test_uninstall() {
  tofuenv install "${v}" || return 1;
  tofuenv uninstall "${v}" || return 1;
  log 'info' 'Confirming uninstall success; an error indicates success:';
-  check_active_version "${v}" && return 1 || return 0;
+  env TOFUENV_AUTO_INSTALL=false "${TOFUENV_ROOT}/bin/tofu" version && return 1 || return 0;


I bypass the helper function here, but this guarantees that tofu fails if the version is not installed.

orbitz · 2024-02-02T13:02:10Z

Hello, would it be possible to close the loop on this soon? I see a test is failing but it looks unrelated to my change.

anastasiiakozlova245 · 2024-02-04T17:03:58Z

Hello @orbitz ! Thank you for continuing working on this feature, I apologize it took so long to review your changes.
The changes LGTM now, except that I still believe that version-name script shouldn't return 'latest' version in case no version is set in any other places.

TOFUENV_VERSION="${TOFUENV_TOFU_DEFAULT_VERSION:-"latest"}";

Could you please not set a version to be 'latest' here by default? This may be confusing for the people who just installed tofuenv and haven't yet installed any tofu versions.

anastasiiakozlova245 · 2024-02-04T17:06:49Z

And also could you please not update main CHANGELOG.md file (it's updated automatically), but add a new file in a .changelog folder, describing your changes? The example can be found here - f5d92f6

lib/tofuenv-version-name.sh

anastasiiakozlova245 · 2024-02-04T17:28:21Z

lib/tofuenv-version-name.sh

+            && log 'debug' "TOFUENV_VERSION specified in TOFUENV_VERSION_FILE: ${TOFUENV_VERSION}";
+        TOFUENV_VERSION_SOURCE="${TOFUENV_VERSION_FILE}";
+    else
+        TOFUENV_VERSION="${TOFUENV_TOFU_DEFAULT_VERSION:-"latest"}";


Continuing my comment in the discussion, I suggest setting the version to an empty string by default, ex.
TOFUENV_VERSION="${TOFUENV_TOFU_DEFAULT_VERSION:-""}";

@anastasiiakozlova245 Won't that mean that tofuenv exec will not have the correct/consistent behaviour?

@orbitz could you please clarify what you mean by not having a consistent behaviour? As stated in the docs, tofuenv install command will still install the latest version if no version is specified, since it doesn't use this particular script. However, tofuenv version-name should show the version of tofu currently installed/set in configs (.opentofu-version file, env variables). If tofuenv is just installed, and no version is set anywhere, why should it print the latest one?

Perhaps I'm confused bu but tofuenv-exec which is called from the tofu wrapper does use this function. I would expect it to have the semantics as tofuenv install. If I change line 18 to :-"" then if no version is found it will look for version "", which will never exist. For example, in main, if I have no version information set:

> ./bin/tofu version cat: /usr/home/orbitz/projects/terrateam/tofuenv/version: No such file or directory Version could not be resolved (set by /usr/home/orbitz/projects/terrateam/tofuenv/version or tofuenv use <version>)

However, with this change:

> ./bin/tofu version version '1.6.1' is not installed (set by /usr/home/orbitz/projects/terrateam/tofuenv/version). Installing now as TOFUENV_AUTO_INSTALL==true Installing OpenTofu v1.6.1 Downloading release tarball from https://github.com/opentofu/opentofu/releases/download/v1.6.1/tofu_1.6.1_freebsd_amd64.zip ##################################################################################################################################################################################################### 100.0% Downloading SHA hash file from https://github.com/opentofu/opentofu/releases/download/v1.6.1/tofu_1.6.1_SHA256SUMS Not instructed to use Local GPG (/usr/home/orbitz/projects/terrateam/tofuenv/use-{gpgv,gnupg}), skipping GnuPG signature verification Installation of tofu v1.6.1 successful. To make this your default version, run 'tofuenv use 1.6.1' OpenTofu v1.6.1 on freebsd_amd64

This is the behaviour I would expect, is this not the behaviour you would expect? If so, what is?

@orbitz I believe when you call ./bin/tofu version, it shouldn't trigger an automatic installation of a latest package. version command is not supposed to install anything, it should just provide an information about the version installed or set anywhere in configs. And if it's not - then returning an error is expected.
@Nmishin what do you think the correct behaviour would be?

I think there are a few things to consider:

Even if you don't think an install should happen, I believe that calling tofu foo should get the same version as when you called tofuenv install. So if tofuenv install defaults to latest if no version information is set, then tofu foo should get that value as well.

As it stands now, if you DO set any version information then tofu foo will install that version if it is not already installed. Line 18 just guarantees that it is matches the tofuenv install version discovery algorithm.

As the code stands now, whether or not tofu foo installs something depends on the value of the auto install variable.

Just bumping this discussion.

orbitz · 2024-02-13T23:47:39Z

Hello, anything I can do to move this change along?

Nmishin · 2024-02-14T11:29:54Z

hello @orbitz i will deep into your changes in a couple of days!

orbitz · 2024-02-22T16:57:36Z

It looks like this pull request is slowly dying. It's been around for weeks and now there is a conflict.

Is there any desire to pull this feature in? If not, we can close this.

Nmishin · 2024-02-26T18:18:56Z

hi @orbitz we discussed internally about this, and we are ok with variable TOFUENV_TOFU_DEFAULT_VERSION,
but the default version should't point to the "latest" because this is not save and will cause an issue if opentofu release incompatible version, and tofuenv automatically update tofu version.

Please update TOFUENV_VERSION="${TOFUENV_TOFU_DEFAULT_VERSION:-"latest"}";
to TOFUENV_VERSION="${TOFUENV_TOFU_DEFAULT_VERSION:-""}"

Nmishin · 2024-02-26T18:19:46Z

.changelog/47.txt

could you please also rename 47.txt to 41.txt - filename should point to pull request number

orbitz · 2024-03-07T12:45:11Z

@Nmishin Depending on where you mean, I don't think setting this to "" has the desired behaviour. See the discussion here: #41 (comment)

Setting the default "" means that tofuenv just breaks with a bad error message when no version information is found. Unless this has been fixed since me writing this PR.

orbitz · 2024-03-07T12:50:20Z

Decided to close this, it looks like tenv has included the functionality I want and it works across every toolchain. Will be moving to tenv in the future

FIX Support default tofu version

b514173

orbitz force-pushed the pro-411-add-opentofu-support branch from 21d4555 to b9b5d85 Compare January 21, 2024 06:36

anastasiiakozlova245 requested changes Jan 21, 2024

View reviewed changes

orbitz force-pushed the pro-411-add-opentofu-support branch 2 times, most recently from f538578 to 09520a4 Compare January 28, 2024 08:51

orbitz force-pushed the pro-411-add-opentofu-support branch 2 times, most recently from e20310a to 9724e73 Compare January 29, 2024 13:26

orbitz commented Jan 29, 2024

View reviewed changes

orbitz requested a review from anastasiiakozlova245 January 29, 2024 21:26

anastasiiakozlova245 reviewed Feb 4, 2024

View reviewed changes

orbitz force-pushed the pro-411-add-opentofu-support branch 2 times, most recently from 254082a to 7818694 Compare February 5, 2024 06:55

orbitz requested a review from anastasiiakozlova245 February 5, 2024 06:55

orbitz force-pushed the pro-411-add-opentofu-support branch from 7818694 to b514173 Compare February 6, 2024 14:55

orbitz mentioned this pull request Feb 17, 2024

Allow setting of a fallback version tofuutils/tenv#64

Closed

Nmishin reviewed Feb 26, 2024

View reviewed changes

.changelog/47.txt

Copy link

Contributor

Nmishin Feb 26, 2024

Choose a reason for hiding this comment

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

could you please also rename 47.txt to 41.txt - filename should point to pull request number

orbitz closed this Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX Support default tofu version #41

FIX Support default tofu version #41

orbitz commented Jan 20, 2024 •

edited

Loading

Nmishin commented Jan 20, 2024

kvendingoldo commented Jan 20, 2024

orbitz commented Jan 21, 2024

orbitz commented Jan 21, 2024

anastasiiakozlova245 Jan 21, 2024 •

edited

Loading

orbitz Jan 21, 2024

anastasiiakozlova245 Jan 21, 2024 •

edited

Loading

anastasiiakozlova245 Jan 21, 2024 •

edited

Loading

orbitz Jan 24, 2024

anastasiiakozlova245 Jan 26, 2024 •

edited

Loading

orbitz Jan 27, 2024

Nmishin commented Jan 27, 2024

orbitz commented Jan 28, 2024

orbitz commented Jan 29, 2024

orbitz Jan 29, 2024

orbitz commented Feb 2, 2024

anastasiiakozlova245 commented Feb 4, 2024 •

edited

Loading

anastasiiakozlova245 commented Feb 4, 2024

anastasiiakozlova245 Feb 4, 2024

orbitz Feb 5, 2024

anastasiiakozlova245 Feb 5, 2024

orbitz Feb 5, 2024

anastasiiakozlova245 Feb 5, 2024 •

edited

Loading

orbitz Feb 5, 2024 •

edited

Loading

orbitz Feb 8, 2024

orbitz commented Feb 13, 2024

Nmishin commented Feb 14, 2024

orbitz commented Feb 22, 2024

Nmishin commented Feb 26, 2024

Nmishin Feb 26, 2024

orbitz commented Mar 7, 2024

orbitz commented Mar 7, 2024

FIX Support default tofu version #41

FIX Support default tofu version #41

Conversation

orbitz commented Jan 20, 2024 • edited Loading

Nmishin commented Jan 20, 2024

kvendingoldo commented Jan 20, 2024

orbitz commented Jan 21, 2024

orbitz commented Jan 21, 2024

anastasiiakozlova245 Jan 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anastasiiakozlova245 Jan 21, 2024 • edited Loading

Choose a reason for hiding this comment

anastasiiakozlova245 Jan 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anastasiiakozlova245 Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nmishin commented Jan 27, 2024

orbitz commented Jan 28, 2024

orbitz commented Jan 29, 2024

Choose a reason for hiding this comment

orbitz commented Feb 2, 2024

anastasiiakozlova245 commented Feb 4, 2024 • edited Loading

anastasiiakozlova245 commented Feb 4, 2024

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

anastasiiakozlova245 Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

orbitz Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbitz commented Feb 13, 2024

Nmishin commented Feb 14, 2024

orbitz commented Feb 22, 2024

Nmishin commented Feb 26, 2024

Choose a reason for hiding this comment

orbitz commented Mar 7, 2024

orbitz commented Mar 7, 2024

orbitz commented Jan 20, 2024 •

edited

Loading

anastasiiakozlova245 Jan 21, 2024 •

edited

Loading

anastasiiakozlova245 Jan 21, 2024 •

edited

Loading

anastasiiakozlova245 Jan 21, 2024 •

edited

Loading

anastasiiakozlova245 Jan 26, 2024 •

edited

Loading

anastasiiakozlova245 commented Feb 4, 2024 •

edited

Loading

anastasiiakozlova245 Feb 5, 2024 •

edited

Loading

orbitz Feb 5, 2024 •

edited

Loading