From fff4a9e8af48fa8141b7f024833d8d2c2b7155ef Mon Sep 17 00:00:00 2001 From: joeduffy Date: Thu, 1 Jun 2017 13:25:49 -0700 Subject: [PATCH] Respond to CR feedback on PR #186 This change responds to some great feedback from @lukehoban on https://github.com/pulumi/lumi/pull/186. Namely: * Relax many assertions in the providers. A failure here is pretty bad because it takes down the entire provider due to fail-fast. That said, I'd rather fail in response to a bug than let it go silently. Nevertheless, a few of them were candidates for dynamic failures. * Use `aws.StringValue` and friends in more places. * Remove a superfluous waitForTableState in DynamoDB. * Fix some erroneous references to `Read` in some `Get` comments. --- lib/aws/provider/dynamodb/table.go | 45 ++++++++++--------- lib/aws/provider/ec2/instance.go | 24 +++++----- lib/aws/provider/ec2/security_group.go | 15 ++++--- .../provider/elasticbeanstalk/application.go | 7 +-- .../elasticbeanstalk/applicationVersion.go | 10 +++-- .../provider/elasticbeanstalk/environment.go | 14 +++--- lib/aws/provider/iam/role.go | 4 +- lib/aws/provider/lambda/function.go | 17 +++---- 8 files changed, 72 insertions(+), 64 deletions(-) diff --git a/lib/aws/provider/dynamodb/table.go b/lib/aws/provider/dynamodb/table.go index 1ff5a35cc..85bdc73d0 100644 --- a/lib/aws/provider/dynamodb/table.go +++ b/lib/aws/provider/dynamodb/table.go @@ -23,6 +23,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" awsdynamodb "github.com/aws/aws-sdk-go/service/dynamodb" + "github.com/pkg/errors" "github.com/pulumi/lumi/pkg/resource" "github.com/pulumi/lumi/pkg/util/contract" "github.com/pulumi/lumi/pkg/util/mapper" @@ -228,10 +229,9 @@ func (p *tableProvider) Create(ctx context.Context, obj *dynamodb.Table) (resour var arn string if resp, err := p.ctx.DynamoDB().CreateTable(create); err != nil { return "", err + } else if resp == nil || resp.TableDescription == nil || resp.TableDescription.TableArn == nil { + return "", errors.New("DynamoDB table created, but AWS did not return an ARN for it") } else { - contract.Assert(resp != nil) - contract.Assert(resp.TableDescription != nil) - contract.Assert(resp.TableDescription.TableArn != nil) arn = *resp.TableDescription.TableArn } @@ -255,33 +255,39 @@ func (p *tableProvider) Get(ctx context.Context, id resource.ID) (*dynamodb.Tabl resp, err := p.ctx.DynamoDB().DescribeTable(&awsdynamodb.DescribeTableInput{TableName: aws.String(name)}) if err != nil { return nil, err + } else if resp == nil || resp.Table == nil { + return nil, errors.New("DynamoDB query returned an empty response or table") } // The object was found, we need to reverse map a bunch of properties into the structure form. - contract.Assert(resp != nil) - contract.Assert(resp.Table != nil) tab := resp.Table var attributes []dynamodb.Attribute for _, attr := range tab.AttributeDefinitions { attributes = append(attributes, dynamodb.Attribute{ - Name: *attr.AttributeName, - Type: dynamodb.AttributeType(*attr.AttributeType), + Name: aws.StringValue(attr.AttributeName), + Type: dynamodb.AttributeType(aws.StringValue(attr.AttributeType)), }) } hashKey, rangeKey := getHashRangeKeys(tab.KeySchema) + if hashKey == nil { + return nil, errors.New("Missing hash key in table schema") + } var gsis *[]dynamodb.GlobalSecondaryIndex if len(tab.GlobalSecondaryIndexes) > 0 { var gis []dynamodb.GlobalSecondaryIndex for _, gsid := range tab.GlobalSecondaryIndexes { hk, rk := getHashRangeKeys(gsid.KeySchema) + if hk == nil { + return nil, errors.New("Missing hash key in table global secondary index") + } gis = append(gis, dynamodb.GlobalSecondaryIndex{ - IndexName: *gsid.IndexName, - HashKey: hk, - ReadCapacity: float64(*gsid.ProvisionedThroughput.ReadCapacityUnits), - WriteCapacity: float64(*gsid.ProvisionedThroughput.WriteCapacityUnits), + IndexName: aws.StringValue(gsid.IndexName), + HashKey: *hk, + ReadCapacity: float64(aws.Int64Value(gsid.ProvisionedThroughput.ReadCapacityUnits)), + WriteCapacity: float64(aws.Int64Value(gsid.ProvisionedThroughput.WriteCapacityUnits)), RangeKey: rk, NonKeyAttributes: aws.StringValueSlice(gsid.Projection.NonKeyAttributes), ProjectionType: dynamodb.ProjectionType(*gsid.Projection.ProjectionType), @@ -291,21 +297,21 @@ func (p *tableProvider) Get(ctx context.Context, id resource.ID) (*dynamodb.Tabl } return &dynamodb.Table{ - HashKey: hashKey, + HashKey: *hashKey, Attributes: attributes, - ReadCapacity: float64(*tab.ProvisionedThroughput.ReadCapacityUnits), - WriteCapacity: float64(*tab.ProvisionedThroughput.WriteCapacityUnits), + ReadCapacity: float64(aws.Int64Value(tab.ProvisionedThroughput.ReadCapacityUnits)), + WriteCapacity: float64(aws.Int64Value(tab.ProvisionedThroughput.WriteCapacityUnits)), RangeKey: rangeKey, TableName: tab.TableName, GlobalSecondaryIndexes: gsis, }, nil } -func getHashRangeKeys(schema []*awsdynamodb.KeySchemaElement) (string, *string) { +func getHashRangeKeys(schema []*awsdynamodb.KeySchemaElement) (*string, *string) { var hashKey *string var rangeKey *string for _, elem := range schema { - switch *elem.KeyType { + switch aws.StringValue(elem.KeyType) { case hashKeyAttribute: hashKey = elem.AttributeName case rangeKeyAttribute: @@ -314,8 +320,7 @@ func getHashRangeKeys(schema []*awsdynamodb.KeySchemaElement) (string, *string) contract.Failf("Unexpected key schema attribute type: %v", *elem.KeyType) } } - contract.Assertf(hashKey != nil, "Expected to discover a hash partition key") - return *hashKey, rangeKey + return hashKey, rangeKey } // InspectChange checks what impacts a hypothetical update will have on the resource's properties. @@ -455,10 +460,6 @@ func (p *tableProvider) Update(ctx context.Context, id resource.ID, return err } } - - if err := p.waitForTableState(name, true); err != nil { - return err - } } return nil } diff --git a/lib/aws/provider/ec2/instance.go b/lib/aws/provider/ec2/instance.go index 2c713841a..ccf7588cd 100644 --- a/lib/aws/provider/ec2/instance.go +++ b/lib/aws/provider/ec2/instance.go @@ -103,15 +103,15 @@ func (p *instanceProvider) Create(ctx context.Context, obj *ec2.Instance) (resou result, err := p.ctx.EC2().RunInstances(create) if err != nil { return "", err + } else if result == nil || len(result.Instances) == 0 { + return "", errors.New("EC2 instance created, but AWS did not return an instance ID for it") } // Get the unique ID from the created instance. - contract.Assert(result != nil) contract.Assert(len(result.Instances) == 1) - inst := result.Instances[0] - contract.Assert(inst != nil) - contract.Assert(inst.InstanceId != nil) - id := *inst.InstanceId + contract.Assert(result.Instances[0] != nil) + contract.Assert(result.Instances[0].InstanceId != nil) + id := aws.StringValue(result.Instances[0].InstanceId) // Before returning that all is okay, wait for the instance to reach the running state. fmt.Fprintf(os.Stdout, "EC2 instance '%v' created; now waiting for it to become 'running'\n", id) @@ -144,26 +144,28 @@ func (p *instanceProvider) Get(ctx context.Context, id resource.ID) (*ec2.Instan // If we are here, we know that there is a reservation that matched; read its fields and populate the object. contract.Assert(len(resp.Reservations) == 1) - resv := resp.Reservations[0] + contract.Assert(resp.Reservations[0] != nil) + contract.Assert(resp.Reservations[0].Instances != nil) contract.Assert(len(resp.Reservations[0].Instances) == 1) - inst := resv.Instances[0] + inst := resp.Reservations[0].Instances[0] var secgrpIDs *[]resource.ID if len(inst.SecurityGroups) > 0 { var ids []resource.ID for _, group := range inst.SecurityGroups { - ids = append(ids, arn.NewEC2SecurityGroupID(idarn.Region, idarn.AccountID, *group.GroupId)) + ids = append(ids, + arn.NewEC2SecurityGroupID(idarn.Region, idarn.AccountID, aws.StringValue(group.GroupId))) } secgrpIDs = &ids } - instanceType := ec2.InstanceType(*inst.InstanceType) + instanceType := ec2.InstanceType(aws.StringValue(inst.InstanceType)) return &ec2.Instance{ - ImageID: *inst.ImageId, + ImageID: aws.StringValue(inst.ImageId), InstanceType: &instanceType, SecurityGroups: secgrpIDs, KeyName: inst.KeyName, - AvailabilityZone: *inst.Placement.AvailabilityZone, + AvailabilityZone: aws.StringValue(inst.Placement.AvailabilityZone), PrivateDNSName: inst.PrivateDnsName, PublicDNSName: inst.PublicDnsName, PrivateIP: inst.PrivateIpAddress, diff --git a/lib/aws/provider/ec2/security_group.go b/lib/aws/provider/ec2/security_group.go index 038ce305e..21c720880 100644 --- a/lib/aws/provider/ec2/security_group.go +++ b/lib/aws/provider/ec2/security_group.go @@ -22,6 +22,7 @@ import ( "github.com/aws/aws-sdk-go/aws" awsec2 "github.com/aws/aws-sdk-go/service/ec2" + "github.com/pkg/errors" "github.com/pulumi/lumi/pkg/resource" "github.com/pulumi/lumi/pkg/util/contract" "github.com/pulumi/lumi/pkg/util/convutil" @@ -94,10 +95,11 @@ func (p *sgProvider) Create(ctx context.Context, obj *ec2.SecurityGroup) (resour result, err := p.ctx.EC2().CreateSecurityGroup(create) if err != nil { return "", err + } else if result == nil || result.GroupId == nil { + return "", errors.New("EC2 security group created, but AWS did not return an ID for it") } - contract.Assert(result != nil) - contract.Assert(result.GroupId != nil) - id := *result.GroupId + + id := aws.StringValue(result.GroupId) // Don't proceed until the security group exists. fmt.Printf("EC2 security group created: %v; waiting for it to become active\n", id) @@ -175,19 +177,18 @@ func (p *sgProvider) Get(ctx context.Context, id resource.ID) (*ec2.SecurityGrou } // If we found one, fetch all the requisite properties and store them on the output. - contract.Assert(len(resp.SecurityGroups) == 1) grp := resp.SecurityGroups[0] var vpcID *resource.ID if grp.VpcId != nil { - vpc := arn.NewEC2VPCID(p.ctx.Region(), p.ctx.AccountID(), *grp.VpcId) + vpc := arn.NewEC2VPCID(p.ctx.Region(), p.ctx.AccountID(), aws.StringValue(grp.VpcId)) vpcID = &vpc } return &ec2.SecurityGroup{ - GroupID: *grp.GroupId, + GroupID: aws.StringValue(grp.GroupId), GroupName: grp.GroupName, - GroupDescription: *grp.Description, + GroupDescription: aws.StringValue(grp.Description), VPC: vpcID, SecurityGroupEgress: createSecurityGroupRulesFromIPPermissions(grp.IpPermissionsEgress), SecurityGroupIngress: createSecurityGroupRulesFromIPPermissions(grp.IpPermissions), diff --git a/lib/aws/provider/elasticbeanstalk/application.go b/lib/aws/provider/elasticbeanstalk/application.go index 637ebfb73..752cd5bdb 100644 --- a/lib/aws/provider/elasticbeanstalk/application.go +++ b/lib/aws/provider/elasticbeanstalk/application.go @@ -53,7 +53,8 @@ type applicationProvider struct { } // Check validates that the given property bag is valid for a resource of the given type. -func (p *applicationProvider) Check(ctx context.Context, obj *elasticbeanstalk.Application) ([]mapper.FieldError, error) { +func (p *applicationProvider) Check(ctx context.Context, + obj *elasticbeanstalk.Application) ([]mapper.FieldError, error) { var failures []mapper.FieldError if name := obj.ApplicationName; name != nil { if len(*name) < minApplicationName { @@ -99,7 +100,7 @@ func (p *applicationProvider) Create(ctx context.Context, obj *elasticbeanstalk. return arn.NewElasticBeanstalkApplicationID(p.ctx.Region(), p.ctx.AccountID(), name), nil } -// Read reads the instance state identified by ID, returning a populated resource object, or an error if not found. +// Get reads the instance state identified by ID, returning a populated resource object, or an error if not found. func (p *applicationProvider) Get(ctx context.Context, id resource.ID) (*elasticbeanstalk.Application, error) { name, err := arn.ParseResourceName(id) if err != nil { @@ -115,7 +116,7 @@ func (p *applicationProvider) Get(ctx context.Context, id resource.ID) (*elastic } contract.Assert(len(resp.Applications) == 1) app := resp.Applications[0] - contract.Assert(*app.ApplicationName == name) + contract.Assert(aws.StringValue(app.ApplicationName) == name) return &elasticbeanstalk.Application{ ApplicationName: app.ApplicationName, Description: app.Description, diff --git a/lib/aws/provider/elasticbeanstalk/applicationVersion.go b/lib/aws/provider/elasticbeanstalk/applicationVersion.go index 858f93c47..e2cb25ba3 100644 --- a/lib/aws/provider/elasticbeanstalk/applicationVersion.go +++ b/lib/aws/provider/elasticbeanstalk/applicationVersion.go @@ -101,7 +101,7 @@ func (p *applicationVersionProvider) Create(ctx context.Context, return arn.NewElasticBeanstalkApplicationVersionID(p.ctx.Region(), p.ctx.AccountID(), appname, versionLabel), nil } -// Read reads the instance state identified by ID, returning a populated resource object, or an error if not found. +// Get reads the instance state identified by ID, returning a populated resource object, or an error if not found. func (p *applicationVersionProvider) Get(ctx context.Context, id resource.ID) (*elasticbeanstalk.ApplicationVersion, error) { idarn, err := arn.ARN(id).Parse() @@ -122,15 +122,17 @@ func (p *applicationVersionProvider) Get(ctx context.Context, } contract.Assert(len(resp.ApplicationVersions) == 1) vers := resp.ApplicationVersions[0] - contract.Assert(*vers.ApplicationName == appname) + contract.Assert(aws.StringValue(vers.ApplicationName) == appname) appid := arn.NewElasticBeanstalkApplication(idarn.Region, idarn.AccountID, appname) - contract.Assert(*vers.VersionLabel == version) + contract.Assert(aws.StringValue(vers.VersionLabel) == version) + s3buck := aws.StringValue(vers.SourceBundle.S3Bucket) + s3key := aws.StringValue(vers.SourceBundle.S3Key) return &elasticbeanstalk.ApplicationVersion{ VersionLabel: vers.VersionLabel, Application: resource.ID(appid), Description: vers.Description, - SourceBundle: arn.NewS3ObjectID(*vers.SourceBundle.S3Bucket, *vers.SourceBundle.S3Key), + SourceBundle: arn.NewS3ObjectID(s3buck, s3key), }, nil } diff --git a/lib/aws/provider/elasticbeanstalk/environment.go b/lib/aws/provider/elasticbeanstalk/environment.go index ea3c4d0c5..3c9ac9839 100644 --- a/lib/aws/provider/elasticbeanstalk/environment.go +++ b/lib/aws/provider/elasticbeanstalk/environment.go @@ -136,7 +136,7 @@ func (p *environmentProvider) Create(ctx context.Context, obj *elasticbeanstalk. return arn.NewElasticBeanstalkEnvironmentID(p.ctx.Region(), p.ctx.AccountID(), appname, name), nil } -// Read reads the instance state identified by ID, returning a populated resource object, or an error if not found. +// Get reads the instance state identified by ID, returning a populated resource object, or an error if not found. func (p *environmentProvider) Get(ctx context.Context, id resource.ID) (*elasticbeanstalk.Environment, error) { appname, envname, err := arn.ParseResourceNamePair(id) if err != nil { @@ -153,9 +153,9 @@ func (p *environmentProvider) Get(ctx context.Context, id resource.ID) (*elastic } else if envresp.Environments == nil || len(envresp.Environments) == 0 { return nil, nil } - contract.Assert(len(envresp.Environments) == 1) // Successfully found the environment, now map all of its properties onto the struct. + contract.Assert(len(envresp.Environments) == 1) env := envresp.Environments[0] if env.CNAME != nil || env.TemplateName != nil || env.Tier != nil { return nil, fmt.Errorf("Properties not yet supported: CNAMEPrefix, TemplateName, Tier") @@ -163,7 +163,7 @@ func (p *environmentProvider) Get(ctx context.Context, id resource.ID) (*elastic var versionLabel *resource.ID if env.VersionLabel != nil { version := arn.NewElasticBeanstalkApplicationVersionID( - p.ctx.Region(), p.ctx.AccountID(), appname, *env.VersionLabel) + p.ctx.Region(), p.ctx.AccountID(), appname, aws.StringValue(env.VersionLabel)) versionLabel = &version } envobj := &elasticbeanstalk.Environment{ @@ -172,7 +172,7 @@ func (p *environmentProvider) Get(ctx context.Context, id resource.ID) (*elastic EnvironmentName: env.EnvironmentName, SolutionStackName: env.SolutionStackName, Version: versionLabel, - EndpointURL: *env.EndpointURL, + EndpointURL: aws.StringValue(env.EndpointURL), } // Next see if there are any configuration option settings and, if so, set them on the return. @@ -186,9 +186,9 @@ func (p *environmentProvider) Get(ctx context.Context, id resource.ID) (*elastic for _, setting := range confresp.ConfigurationSettings { for _, option := range setting.OptionSettings { options = append(options, elasticbeanstalk.OptionSetting{ - Namespace: *option.Namespace, - OptionName: *option.OptionName, - Value: *option.Value, + Namespace: aws.StringValue(option.Namespace), + OptionName: aws.StringValue(option.OptionName), + Value: aws.StringValue(option.Value), }) } } diff --git a/lib/aws/provider/iam/role.go b/lib/aws/provider/iam/role.go index 19cb2b992..dfd85ebae 100644 --- a/lib/aws/provider/iam/role.go +++ b/lib/aws/provider/iam/role.go @@ -146,7 +146,7 @@ func (p *roleProvider) Get(ctx context.Context, id resource.ID) (*iam.Role, erro if len(getpols.AttachedPolicies) > 0 { var policies []awscommon.ARN for _, policy := range getpols.AttachedPolicies { - policies = append(policies, awscommon.ARN(*policy.PolicyArn)) + policies = append(policies, awscommon.ARN(aws.StringValue(policy.PolicyArn))) } managedPolicies = &policies } @@ -156,7 +156,7 @@ func (p *roleProvider) Get(ctx context.Context, id resource.ID) (*iam.Role, erro Path: role.Path, RoleName: role.RoleName, ManagedPolicyARNs: managedPolicies, - ARN: awscommon.ARN(*role.Arn), + ARN: awscommon.ARN(aws.StringValue(role.Arn)), }, nil } diff --git a/lib/aws/provider/lambda/function.go b/lib/aws/provider/lambda/function.go index e413ce1e9..1fcab4dce 100644 --- a/lib/aws/provider/lambda/function.go +++ b/lib/aws/provider/lambda/function.go @@ -23,6 +23,7 @@ import ( "github.com/aws/aws-sdk-go/aws" awslambda "github.com/aws/aws-sdk-go/service/lambda" + "github.com/pkg/errors" "github.com/pulumi/lumi/pkg/resource" "github.com/pulumi/lumi/pkg/util/contract" "github.com/pulumi/lumi/pkg/util/convutil" @@ -153,9 +154,9 @@ func (p *funcProvider) Create(ctx context.Context, obj *lambda.Function) (resour return false, nil // retry the condition. } return false, err + } else if resp == nil || resp.FunctionArn == nil { + return false, errors.New("Lambda Function created, but AWS did not respond with an ARN") } - contract.Assert(resp != nil) - contract.Assert(resp.FunctionArn != nil) arn = resource.ID(*resp.FunctionArn) return true, nil }, @@ -177,7 +178,7 @@ func (p *funcProvider) Create(ctx context.Context, obj *lambda.Function) (resour return arn, nil } -// Read reads the instance state identified by ID, returning a populated resource object, or an error if not found. +// Get reads the instance state identified by ID, returning a populated resource object, or an error if not found. func (p *funcProvider) Get(ctx context.Context, id resource.ID) (*lambda.Function, error) { name, err := arn.ParseResourceName(id) if err != nil { @@ -195,7 +196,7 @@ func (p *funcProvider) Get(ctx context.Context, id resource.ID) (*lambda.Functio contract.Assert(funcresp != nil) contract.Assert(funcresp.Code != nil) // TODO: unclear if we need to consult RepositoryType. - code := resource.NewURIArchive(*funcresp.Code.Location) + code := resource.NewURIArchive(aws.StringValue(funcresp.Code.Location)) // Deserialize all configuration properties into prompt objects. config := funcresp.Configuration @@ -207,11 +208,11 @@ func (p *funcProvider) Get(ctx context.Context, id resource.ID) (*lambda.Functio } return &lambda.Function{ - ARN: awscommon.ARN(*config.FunctionArn), + ARN: awscommon.ARN(aws.StringValue(config.FunctionArn)), Code: code, - Handler: *config.Handler, - Role: resource.ID(*config.Role), - Runtime: lambda.Runtime(*config.Runtime), + Handler: aws.StringValue(config.Handler), + Role: resource.ID(aws.StringValue(config.Role)), + Runtime: lambda.Runtime(aws.StringValue(config.Runtime)), FunctionName: config.FunctionName, Description: config.Description, Environment: env,