first part of code review comments

This commit is contained in:
Megan Bang 2022-08-30 17:01:44 -05:00
parent 7e5b7b283e
commit de8bd5826f
16 changed files with 107 additions and 95 deletions

2
go.mod
View File

@ -40,7 +40,7 @@ require (
github.com/hashicorp/go-multierror v1.1.1
github.com/hashicorp/go-plugin v1.4.3
github.com/hashicorp/go-retryablehttp v0.7.1
github.com/hashicorp/go-tfe v1.8.1-0.20220826155809-b28335218b42
github.com/hashicorp/go-tfe v1.9.0
github.com/hashicorp/go-uuid v1.0.3
github.com/hashicorp/go-version v1.6.0
github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f

4
go.sum
View File

@ -375,8 +375,8 @@ github.com/hashicorp/go-slug v0.10.0/go.mod h1:Ib+IWBYfEfJGI1ZyXMGNbu2BU+aa3Dzu4
github.com/hashicorp/go-sockaddr v1.0.0 h1:GeH6tui99pF4NJgfnhp+L6+FfobzVW3Ah46sLo0ICXs=
github.com/hashicorp/go-sockaddr v1.0.0/go.mod h1:7Xibr9yA9JjQq1JpNB2Vw7kxv8xerXegt+ozgdvDeDU=
github.com/hashicorp/go-syslog v1.0.0/go.mod h1:qPfqrKkXGihmCqbJM2mZgkZGvKG1dFdvsLplgctolz4=
github.com/hashicorp/go-tfe v1.8.1-0.20220826155809-b28335218b42 h1:3Ejb02U7WAl9NBRYArj8Hyw0dvHEB5NO1AsX06wbDq4=
github.com/hashicorp/go-tfe v1.8.1-0.20220826155809-b28335218b42/go.mod h1:uSWi2sPw7tLrqNIiASid9j3SprbbkPSJ/2s3X0mMemg=
github.com/hashicorp/go-tfe v1.9.0 h1:jkmyo7WKNA7gZDegG5imndoC4sojWXhqMufO+KcHqrU=
github.com/hashicorp/go-tfe v1.9.0/go.mod h1:uSWi2sPw7tLrqNIiASid9j3SprbbkPSJ/2s3X0mMemg=
github.com/hashicorp/go-uuid v1.0.0/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro=
github.com/hashicorp/go-uuid v1.0.1/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro=
github.com/hashicorp/go-uuid v1.0.2/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro=

View File

@ -33,8 +33,6 @@ import (
type State struct {
mu sync.Mutex
// Client Client
// We track two pieces of meta data in addition to the state itself:
//
// lineage - the state's unique ID
@ -142,7 +140,7 @@ func (s *State) WriteState(state *states.State) error {
return nil
}
// statemgr.Persister impl.
// PersistState uploads a snapshot of the latest state as a StateVersion to Terraform Cloud
func (s *State) PersistState(schemas *terraform.Schemas) error {
s.mu.Lock()
defer s.mu.Unlock()

View File

@ -5,10 +5,8 @@ import (
"io/ioutil"
"testing"
tfe "github.com/hashicorp/go-tfe"
"github.com/hashicorp/terraform/internal/states/statefile"
"github.com/hashicorp/go-tfe"
"github.com/hashicorp/terraform/internal/states/statemgr"
)
@ -100,10 +98,6 @@ func TestState(t *testing.T) {
jsonStateOutputs := []byte(`
{
"version": 4,
"terraform_version": "1.3.0",
"serial": 1,
"lineage": "backend-change",
"outputs": {
"foo": {
"type": "string",
@ -136,3 +130,66 @@ func TestState(t *testing.T) {
t.Fatalf("expected empty state, got: %q", string(p.Data))
}
}
func TestCloudLocks(t *testing.T) {
back, bCleanup := testBackendWithName(t)
defer bCleanup()
a, err := back.StateMgr(testBackendSingleWorkspaceName)
if err != nil {
t.Fatalf("expected no error, got %v", err)
}
b, err := back.StateMgr(testBackendSingleWorkspaceName)
if err != nil {
t.Fatalf("expected no error, got %v", err)
}
lockerA, ok := a.(statemgr.Locker)
if !ok {
t.Fatal("client A not a statemgr.Locker")
}
lockerB, ok := b.(statemgr.Locker)
if !ok {
t.Fatal("client B not a statemgr.Locker")
}
infoA := statemgr.NewLockInfo()
infoA.Operation = "test"
infoA.Who = "clientA"
infoB := statemgr.NewLockInfo()
infoB.Operation = "test"
infoB.Who = "clientB"
lockIDA, err := lockerA.Lock(infoA)
if err != nil {
t.Fatal("unable to get initial lock:", err)
}
_, err = lockerB.Lock(infoB)
if err == nil {
lockerA.Unlock(lockIDA)
t.Fatal("client B obtained lock while held by client A")
}
if _, ok := err.(*statemgr.LockError); !ok {
t.Errorf("expected a LockError, but was %t: %s", err, err)
}
if err := lockerA.Unlock(lockIDA); err != nil {
t.Fatal("error unlocking client A", err)
}
lockIDB, err := lockerB.Lock(infoB)
if err != nil {
t.Fatal("unable to obtain lock from client B")
}
if lockIDB == lockIDA {
t.Fatalf("duplicate lock IDs: %q", lockIDB)
}
if err = lockerB.Unlock(lockIDB); err != nil {
t.Fatal("error unlocking client B:", err)
}
}

View File

@ -11,8 +11,6 @@ import (
"testing"
"time"
"github.com/hashicorp/terraform/internal/states/statemgr"
tfe "github.com/hashicorp/go-tfe"
svchost "github.com/hashicorp/terraform-svchost"
"github.com/hashicorp/terraform-svchost/auth"
@ -444,54 +442,3 @@ func testVariables(s terraform.ValueSourceType, vs ...string) map[string]backend
}
return vars
}
func TestCloudLocks(t *testing.T, a, b statemgr.Full) {
lockerA, ok := a.(statemgr.Locker)
if !ok {
t.Fatal("client A not a statemgr.Locker")
}
lockerB, ok := b.(statemgr.Locker)
if !ok {
t.Fatal("client B not a statemgr.Locker")
}
infoA := statemgr.NewLockInfo()
infoA.Operation = "test"
infoA.Who = "clientA"
infoB := statemgr.NewLockInfo()
infoB.Operation = "test"
infoB.Who = "clientB"
lockIDA, err := lockerA.Lock(infoA)
if err != nil {
t.Fatal("unable to get initial lock:", err)
}
_, err = lockerB.Lock(infoB)
if err == nil {
lockerA.Unlock(lockIDA)
t.Fatal("client B obtained lock while held by client A")
}
if _, ok := err.(*statemgr.LockError); !ok {
t.Errorf("expected a LockError, but was %t: %s", err, err)
}
if err := lockerA.Unlock(lockIDA); err != nil {
t.Fatal("error unlocking client A", err)
}
lockIDB, err := lockerB.Lock(infoB)
if err != nil {
t.Fatal("unable to obtain lock from client B")
}
if lockIDB == lockIDA {
t.Fatalf("duplicate lock IDs: %q", lockIDB)
}
if err = lockerB.Unlock(lockIDB); err != nil {
t.Fatal("error unlocking client B:", err)
}
}

View File

@ -6,13 +6,19 @@ import (
)
const failedToLoadSchemasMessage = `
Terraform failed to load schemas, which will in turn affect its ability to generate the
external JSON state file. This will not have any adverse effects on Terraforms ability
to maintain state information, but may have adverse effects on any external integrations
relying on this format. The file should be created on the next successful "terraform apply"
however, historic state information may be missing if the affected integration relies on that
Warning: Failed to update data for external integrations
%s
Terraform was unable to generate a description of the updated
state for use with external integrations in Terraform Cloud.
Any integrations configured for this workspace which depend on
information from the state may not work correctly when using the
result of this action.
This problem occurs when Terraform cannot read the schema for
one or more of the providers used in the state. The next successful
apply will correct the problem by re-generating the JSON description
of the state:
terraform apply
`
func isCloudMode(b backend.Enhanced) bool {

View File

@ -251,9 +251,9 @@ func (c *ImportCommand) Run(args []string) int {
// Get schemas, if possible, before writing state
var schemas *terraform.Schemas
if isCloudMode(b) {
schemas, diags = c.GetSchemas(newState, nil)
schemas, diags = c.MaybeGetSchemas(newState, nil)
if diags.HasErrors() {
c.Ui.Warn(fmt.Sprintf(failedToLoadSchemasMessage, err))
c.Ui.Warn(failedToLoadSchemasMessage)
}
}

View File

@ -14,9 +14,6 @@ import (
"strings"
"time"
"github.com/hashicorp/terraform/internal/configs"
"github.com/hashicorp/terraform/internal/states"
plugin "github.com/hashicorp/go-plugin"
"github.com/hashicorp/terraform-svchost/disco"
"github.com/mitchellh/cli"
@ -30,11 +27,13 @@ import (
"github.com/hashicorp/terraform/internal/command/views"
"github.com/hashicorp/terraform/internal/command/webbrowser"
"github.com/hashicorp/terraform/internal/command/workdir"
"github.com/hashicorp/terraform/internal/configs"
"github.com/hashicorp/terraform/internal/configs/configload"
"github.com/hashicorp/terraform/internal/getproviders"
legacy "github.com/hashicorp/terraform/internal/legacy/terraform"
"github.com/hashicorp/terraform/internal/providers"
"github.com/hashicorp/terraform/internal/provisioners"
"github.com/hashicorp/terraform/internal/states"
"github.com/hashicorp/terraform/internal/terminal"
"github.com/hashicorp/terraform/internal/terraform"
"github.com/hashicorp/terraform/internal/tfdiags"
@ -783,8 +782,12 @@ func (m *Meta) checkRequiredVersion() tfdiags.Diagnostics {
return nil
}
// GetSchemas loads and returns the schemas
func (c *Meta) GetSchemas(state *states.State, config *configs.Config) (*terraform.Schemas, tfdiags.Diagnostics) {
// MaybeGetSchemas attempts to load and return the schemas
// If there is not enough information to return the schemas,
// it could potentially return nil without errors. It is the
// responsibility of the caller to handle the lack of schema
// information accordingly
func (c *Meta) MaybeGetSchemas(state *states.State, config *configs.Config) (*terraform.Schemas, tfdiags.Diagnostics) {
var diags tfdiags.Diagnostics
path, err := os.Getwd()

View File

@ -438,9 +438,10 @@ func (m *Meta) backendMigrateState_s_s(opts *backendMigrateOpts) error {
return fmt.Errorf(strings.TrimSpace(errBackendStateCopy),
opts.SourceType, opts.DestinationType, err)
}
// Fetching schemas during init might be more of a hassle than we want to attempt
// in the case that we're migrating to TFC backend, the initial JSON state won't
// be generated and stored.
// The backend is currently handled before providers are installed during init,
// so requiring schemas here could lead to a catch-22 where it requires some manual
// intervention to proceed far enough for provider installation. To avoid this,
// when migrating to TFC backend, the initial JSON varient of state won't be generated and stored.
if err := destinationState.PersistState(nil); err != nil {
return fmt.Errorf(strings.TrimSpace(errBackendStateCopy),
opts.SourceType, opts.DestinationType, err)

View File

@ -110,7 +110,7 @@ func (c *ShowCommand) show(path string) (*plans.Plan, *statefile.File, *configs.
// Get schemas, if possible
if config != nil || stateFile != nil {
schemas, diags = c.GetSchemas(stateFile.State, config)
schemas, diags = c.MaybeGetSchemas(stateFile.State, config)
if diags.HasErrors() {
return plan, stateFile, config, schemas, diags
}

View File

@ -397,9 +397,9 @@ func (c *StateMvCommand) Run(args []string) int {
// Get schemas, if possible, before writing state
var schemas *terraform.Schemas
if isCloudMode(b) {
schemas, diags = c.GetSchemas(stateTo, nil)
schemas, diags = c.MaybeGetSchemas(stateTo, nil)
if diags.HasErrors() {
c.Ui.Warn(fmt.Sprintf(failedToLoadSchemasMessage, err))
c.Ui.Warn(failedToLoadSchemasMessage)
}
}

View File

@ -134,9 +134,9 @@ func (c *StatePushCommand) Run(args []string) int {
var schemas *terraform.Schemas
if isCloudMode(b) {
var diags tfdiags.Diagnostics
schemas, diags = c.GetSchemas(srcStateFile.State, nil)
schemas, diags = c.MaybeGetSchemas(srcStateFile.State, nil)
if diags.HasErrors() {
c.Ui.Warn(fmt.Sprintf(failedToLoadSchemasMessage, err))
c.Ui.Warn(failedToLoadSchemasMessage)
}
}

View File

@ -172,9 +172,9 @@ func (c *StateReplaceProviderCommand) Run(args []string) int {
// Get schemas, if possible, before writing state
var schemas *terraform.Schemas
if isCloudMode(b) {
schemas, diags = c.GetSchemas(state, nil)
schemas, diags = c.MaybeGetSchemas(state, nil)
if diags.HasErrors() {
c.Ui.Warn(fmt.Sprintf(failedToLoadSchemasMessage, err))
c.Ui.Warn(failedToLoadSchemasMessage)
}
}

View File

@ -122,9 +122,9 @@ func (c *StateRmCommand) Run(args []string) int {
// Get schemas, if possible, before writing state
var schemas *terraform.Schemas
if isCloudMode(b) {
schemas, diags = c.GetSchemas(state, nil)
schemas, diags = c.MaybeGetSchemas(state, nil)
if diags.HasErrors() {
c.Ui.Warn(fmt.Sprintf(failedToLoadSchemasMessage, err))
c.Ui.Warn(failedToLoadSchemasMessage)
}
}

View File

@ -130,9 +130,9 @@ func (c *TaintCommand) Run(args []string) int {
// Get schemas, if possible, before writing state
var schemas *terraform.Schemas
if isCloudMode(b) {
schemas, diags = c.GetSchemas(state, nil)
schemas, diags = c.MaybeGetSchemas(state, nil)
if diags.HasErrors() {
c.Ui.Warn(fmt.Sprintf(failedToLoadSchemasMessage, err))
c.Ui.Warn(failedToLoadSchemasMessage)
}
}

View File

@ -169,9 +169,9 @@ func (c *UntaintCommand) Run(args []string) int {
// Get schemas, if possible, before writing state
var schemas *terraform.Schemas
if isCloudMode(b) {
schemas, diags = c.GetSchemas(state, nil)
schemas, diags = c.MaybeGetSchemas(state, nil)
if diags.HasErrors() {
c.Ui.Warn(fmt.Sprintf(failedToLoadSchemasMessage, err))
c.Ui.Warn(failedToLoadSchemasMessage)
}
}