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

tfinstall package #8

Merged
merged 3 commits into from
Jul 3, 2020
Merged

tfinstall package #8

merged 3 commits into from
Jul 3, 2020

Conversation

kmoe
Copy link
Member

@kmoe kmoe commented Jun 23, 2020

Adds tfinstall.Find(...*findOption) (string, error), which locates or downloads a terraform binary according to the strategies provided. These are:

ExactPath(execPath string)

Path to an existing terraform binary on the local filesystem.

LookPath()

Attempt to locate a terraform binary on the local filesystem using exec.LookPath().

LatestVersion(installDir string)

Attempt to download the latest version of terraform and save it to the provided installDir (if blank, a temporary directory will be created).

ExactVersion(tfVersion string, installDir string)

As above, but will only download the exact provided version of terraform.

Example

tfPath, err := tfinstall.Find(tfinstall.LookPath(), tfinstall.LatestVersion(""))

First, tfinstall will attempt to locate terraform on the local system path. If found, this path will be returned as tfPath.

If the executable is not found on the local system path, tfinstall will download the latest version of terraform and return the path to the downloaded executable as tfPath.

An error is only returned if all provided strategies fail.

@kmoe kmoe requested review from a team and removed request for a team June 23, 2020 19:13
@kmoe
Copy link
Member Author

kmoe commented Jun 24, 2020

WIP, reconsidering API

@paultyng paultyng marked this pull request as draft June 25, 2020 17:27
@kmoe kmoe force-pushed the install-terraform branch from cc5140d to 23a6914 Compare July 1, 2020 15:29
@kmoe kmoe requested review from paultyng and a team July 1, 2020 15:40
@kmoe kmoe marked this pull request as ready for review July 1, 2020 15:40
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

From high-level-API perspective I wonder if ExactPath and LookPath belong to this (tfinstall) package.

Would it make sense to decouple those into tfdisco and let the consumers decide about whether they want to couple them and how? At least in the language server I suspect I'd certainly use the discovery part, but far less likely the installation.

tfinstall/tfinstall.go Outdated Show resolved Hide resolved
tfinstall/tfinstall.go Outdated Show resolved Hide resolved
tfinstall/tfinstall.go Outdated Show resolved Hide resolved
tfinstall/tfinstall.go Outdated Show resolved Hide resolved
tfinstall/tfinstall.go Outdated Show resolved Hide resolved
tfinstall/tfinstall.go Outdated Show resolved Hide resolved
@kmoe kmoe force-pushed the install-terraform branch from abd0af9 to 32a788e Compare July 1, 2020 19:06
@kmoe kmoe changed the title tfinstall package tfdisco package Jul 1, 2020
@kmoe kmoe force-pushed the install-terraform branch from 32a788e to c76758d Compare July 1, 2020 19:16
@kmoe
Copy link
Member Author

kmoe commented Jul 1, 2020

Thanks for the tfdisco name, @radeksimko . I love it. I think it's a great name for the package overall, and the consumer can decide which parts of it to use. The language server need only run tfdisco.Find(tfdisco.LookPath()) (or perhaps tfdisco.Find(tfdisco.ExactPath("/special/path"), tfdisco.LookPath()) if you want to allow users to force the LS to prefer a particular binary by placing it at "/special/path").

I've simplified the error handling logic along the lines suggested by @paultyng. Errors encountered during each strategy will now cause Find() to error overall, except in the following two expected cases:

  1. tfdisco.LookPath(): if a terraform executable is not found, fall through to the next strategy. (Of course if LookPath() is the final strategy, this means Find() will error overall, as it cannot return a valid path to an executable).

  2. tfdisco.ExactPath(path): We now stat the provided path, and fall through without error if it does not exist. This is based on how I imagine library consumers might use ExactPath() - to provide one or two default paths up front, in case Terraform is already installed in a well-known location.

@kmoe kmoe requested a review from radeksimko July 1, 2020 19:17
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Sorry for the misunderstanding, but I was rather suggesting decoupling of "discovery" functions (i.e. LookPath and ExactPath) entirely to the tfdisco package and leaving the installation functions (LatestVersion, ExactVersion) in tfinstall.

I understand that the test framework needs the functionality "find or install", but I think that decision logic (whether to install) should either stay in the test framework or be decoupled to yet another layer if it really stays here.

e.g. I may want to do discovery earlier in the workflow, but install later, or something like that and the current coupling would make me do the discovery again, which seems unnecessary?

@kmoe
Copy link
Member Author

kmoe commented Jul 2, 2020

e.g. I may want to do discovery earlier in the workflow, but install later, or something like that and the current coupling would make me do the discovery again, which seems unnecessary?

I don't quite understand your use case, and I'm reluctant to split into two packages here because I see this package as a natural grouping of Find() and its findOptions. Could you describe a bit more the LS workflow, and why it wouldn't be sufficient to do tfdisco.Find(tfdisco.LookPath()) earlier, and tfdisco.Find(tfdisco.LatestVersion()) (for instance) later?

@radeksimko
Copy link
Member

Could you describe a bit more the LS workflow

The LS currently doesn't install Terraform, and I'm not sure when it will or how yet, but I'm assuming that we'd prompt the user to confirm the installation. In order to do that we'll need more flexibility around composing different parts of the library.

I imagine the workflow would be something like

  1. Is there any Terraform installed? (filesystem discovery)
  2. What are the version constraints for the code being edited? (LS will parse terraform blocks)
  3. Is that discovered version compatible with these constraints?
  4. If not, what is the latest available version within these constraints? (Checkpoint discovery)
  5. Present a prompt to the user "No terraform found in PATH, do you want to install X.Y.Z for /path/to/dir?"
  6. Does the user consent with the installation of version X.Y.Z?
    • they may cancel, and install Terraform via APT/Homebrew/... and/or make sure their OS-wide installation is discoverable in $PATH
  7. Install version X.Y.Z. (installation)

why it wouldn't be sufficient to do tfdisco.Find(tfdisco.LookPath()) earlier, and tfdisco.Find(tfdisco.LatestVersion()) (for instance) later?

It's not that it's impossible, it's more the UX/API design that makes it difficult to expand and support more use cases, because it assumes that everyone will need both discovery and installation at the same time, which I don't think is true.

Terraform can be installed outside of terraform-exec entirely and that method of installation may even be preferred in situations where there is practical access to OS package manager and user is in control (as opposed to CI or other automation-related scenarios).

The discovery runs terraform version, but doesn't provide the output to the consumer, making them run it again.

tfdisco.LatestVersion()(without more closely inspecting the argument names or implementation) makes me think it will just discover the latest version and return it either as go-version.Version or as a string. That is why I think it belongs to a different package, or at least should be renamed to InstallLatestVersion and ExactVersion to InstallExactVersion.


I don't want to overburden you with all these different scenarios at such early stage though, because I think the initial implementation can be simple and doesn't need to satisfy all different use cases, but I do think that we should leave some room in the API so we can tweak it more easily to cater more use cases.

With that in mind I'd be happy for the "find or install" logic to remain in the library, but it should be clear(er) upfront that this is what it does.

Maybe you could turn Find into FirstExecutablePath and *findOption into an interface

type ExecutablePathFinder interface {
    ExecPath() string
}

and make each installer and discoverer comply with that interface?

Then you'd use it like this:

FirstExecutablePath(
    tfdisco.ExactPath("/path/to/tf"),
    tfdisco.LookPath(),
    tfinstall.NewInstaller("latest"),
    tfinstall.NewInstaller("0.12.28"))

which leaves room for API surface growth on installer side, discovery side and the "coupling" side.

@kmoe kmoe force-pushed the install-terraform branch from c76758d to ed7cc6b Compare July 2, 2020 21:43
@kmoe kmoe changed the title tfdisco package tfinstall package Jul 2, 2020
@kmoe kmoe force-pushed the install-terraform branch 2 times, most recently from 0459d96 to 2fd1beb Compare July 2, 2020 22:08
@kmoe
Copy link
Member Author

kmoe commented Jul 2, 2020

I've reverted the package name to tfinstall, because the majority of use cases are concerned with installing or otherwise obtaining a Terraform binary. The options pattern is now similar to that in tfexec itself, along the lines described by @radeksimko. This should make the usage clearer, and also improve the structure of the GoDoc, so tfinstall.LatestVersion() etc are clearly intended as function options (see the GoDoc for tfexec, https://godoc.org/github.com/hashicorp/terraform-exec/tfexec).

Copy link
Contributor

@paultyng paultyng left a comment

Choose a reason for hiding this comment

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

LGTM, you may want to add some testing around pre-release versions.

@kmoe kmoe force-pushed the install-terraform branch from 2fd1beb to cace9f2 Compare July 3, 2020 14:49
@kmoe kmoe merged commit 22dfd35 into master Jul 3, 2020
@kmoe kmoe deleted the install-terraform branch July 3, 2020 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants