Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

pkg/terraform: improvements #1027

Merged
merged 7 commits into from
Oct 5, 2020
Merged
6 changes: 2 additions & 4 deletions pkg/terraform/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -534,10 +534,8 @@ func (ex *Executor) checkVersion() error {
return fmt.Errorf("checking Terraform version: %w", err)
}

constraints, err := version.NewConstraint(requiredVersion)
if err != nil {
return fmt.Errorf("checking Terraform version: %w", err)
}
// requiredVersion is const, so we test it in unit tests.
constraints, _ := version.NewConstraint(requiredVersion)
Copy link
Member

Choose a reason for hiding this comment

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

we can log it in debug mode? So we don't lose track of it altogether.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? It is covered by unit tests, running exactly the same code.

Copy link
Member

Choose a reason for hiding this comment

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

It's best to panic in such case, if this error should be found during development itself.


if !constraints.Check(v) {
return fmt.Errorf("version '%s' of Terraform not supported. Needed %s", v, constraints)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,28 +1,34 @@
//+build e2e
invidian marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Should e2e tests sit in test dir? Mixing unit tests and e2e tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO, those are package-scoped tests, so I think it's fine if they sit in this directory. They are marked as e2e, as they require external dependency to pass (terraform binary).


package terraform
package terraform_test

import (
"io/ioutil"
"os"
"strings"
"testing"

"github.com/kinvolk/lokomotive/pkg/terraform"
)

func executor(t *testing.T) *Executor {
func executor(t *testing.T) *terraform.Executor {
tmpDir, err := ioutil.TempDir("", "lokoctl-tests-")
if err != nil {
t.Fatalf("Creating tmp dir should succeed, got: %v", err)
}

defer os.RemoveAll(tmpDir)
t.Cleanup(func() {
if err := os.RemoveAll(tmpDir); err != nil {
t.Logf("Removing directory %q: %v", tmpDir, err)
}
})

conf := Config{
conf := terraform.Config{
Verbose: false,
WorkingDir: tmpDir,
}

ex, err := NewExecutor(conf)
ex, err := terraform.NewExecutor(conf)
if err != nil {
t.Fatalf("Creating new executor should succeed, got: %v", err)
}
Expand Down
27 changes: 27 additions & 0 deletions pkg/terraform/executor_internal_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Copyright 2020 The Lokomotive Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package terraform
invidian marked this conversation as resolved.
Show resolved Hide resolved

import (
"testing"

"github.com/hashicorp/go-version"
)

func TestRequiredVersion(t *testing.T) {
if _, err := version.NewConstraint(requiredVersion); err != nil {
t.Fatalf("requiredVersion const must be valid version constraint, got: %v", err)
}
}