- Notifications
You must be signed in to change notification settings - Fork928
feat: Download default terraform version when minor version mismatches#1775
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
provisioner/terraform/serve.go Outdated
| // If the "coder" binary is in the same directory as | ||
| // the "terraform" binary, "terraform" is returned. | ||
| // | ||
| // We must resolve the absolute path for other processes | ||
| // to execute this properly! | ||
| absoluteBinary, err := filepath.Abs(binaryPath) | ||
| if err != nil { | ||
| return xerrors.Errorf("absolute: %w", err) | ||
| } | ||
| // Checking the installed version of Terraform. | ||
| output, err := exec.Command(absoluteBinary, "version").Output() | ||
| if err != nil { | ||
| return xerrors.Errorf("terraform version: %w", err) | ||
| } | ||
| // The output for `terraform version` is: | ||
| // Terraform v1.2.1 | ||
| // on linux_amd64 | ||
| versionRegex := regexp.MustCompile("Terraform v(.+)\n?.*") | ||
| match := versionRegex.FindStringSubmatch(string(output)) | ||
| if match != nil { | ||
| // match[0] is the entire string. | ||
| // match[1] is the matched substring. | ||
| version := match[1] | ||
| terraformMinorVersion := strings.Join(strings.Split(terraformVersion, versionDelimiter)[:2], versionDelimiter) | ||
| if !strings.HasPrefix(version, terraformMinorVersion) { | ||
| downloadTerraform = true | ||
| } | ||
| } else { | ||
| // Download the required Terraform version when unable to determine the existing one. | ||
| downloadTerraform = true | ||
| } | ||
| if !downloadTerraform { | ||
| options.BinaryPath = absoluteBinary | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Theterraform-exec library already handles most of this for us!
https://github.com/coder/coder/blob/main/provisioner/terraform/provision.go#L63-L70
What is the status of this PR? |
@ammario Got stuck in limbo as I moved onto some frontend changes. I am back to addressing the comments on it this week; we should be able to get it in by EOW. |
07c6480 to91fd183Compare91fd183 toccddf23CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good, beautiful tests 😉!
| t.Cleanup(func() { | ||
| t.Setenv("PATH", pathVariable) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| t.Cleanup(func() { | |
| t.Setenv("PATH",pathVariable) | |
| }) |
I don't think this is needed?t.Setenv already defines a cleanup function that restores the environment in the earlier step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Removed it along with the top levelt.Parallel since this test was conflicting with some other tests.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
cad1205 to7964593CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice!
provisioner/terraform/serve.go Outdated
| @@ -41,33 +43,54 @@ type ServeOptions struct { | |||
| Logger slog.Logger | |||
| } | |||
| func getAbsoluteBinaryPath(ctx context.Context) (string, bool) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Following Go idioms, thisget prefix could be removed!
ed969f2 to4580e8cCompare4580e8c to744052fCompare744052f to6b84a73Compare
Uh oh!
There was an error while loading.Please reload this page.
This PR downloads the default Terraform version when the minor version of the already installed Terraform is different.
Subtasks
terraform version.Fixes#1743