New Resource: aws_gamelift_build #2843

radeksimko · 2018-01-03T09:51:53Z

I want to take a look into the ZIP file to see if there's any way to bring the size down from 15M, but otherwise this is ready for review.

The ZIP file was basically created by following steps in https://github.com/awslabs/aws-gamelift-sample/blob/master/deployment/deployment.md#gamelift----game-server-build--fleet-creation
and it unfortunately contains a few binaries which practically can't be compressed.

The build resource here practically doesn't require a ZIP file with fully working game, but aws_gamelift_fleet (for which I already have a PR in a queue) will.

Import

The main reason I decided not to implement import is because DescribeBuild doesn't return storage_location which is ForceNew, so having such empty field in the state would result in recreation.

Test Results

$ make testacc TESTARGS='-run=TestAccAWSGamelift'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSGamelift -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSGameliftBuild_basic
--- PASS: TestAccAWSGameliftBuild_basic (313.15s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	313.202s

stack72 · 2018-01-03T15:25:59Z

@radeksimko can't this zip be hosted on a public S3 bucket and then downloaded as part of the test?

radeksimko · 2018-01-03T17:06:15Z

Ideally I'd like the whole test suite to be self-sustainable, not relying on precisely named files in a precisely named S3 bucket. It makes the test a bit fragile.

radeksimko · 2018-01-08T12:48:11Z

This is the contents of the ZIP file:

$ du -h ./*
100K	./GomokuServer.exe
1000K	./aws-cpp-sdk-core.dll
1.9M	./aws-cpp-sdk-gamelift-server.dll
688K	./aws-cpp-sdk-sqs.dll
4.0K	./config.ini
4.0K	./install.bat
 14M	./vc_redist.x64.exe

where the last binary (which was downloaded from MSFT's website per linked tutorial) in the list occupies the majority of the space. This leads me to a conclusion that we can't unfortunately make it much smaller.

To put this into context here's usage of the whole repo:

du -s -h ./*
 56K	./CHANGELOG.md
4.0K	./GNUmakefile
 20K	./LICENSE
4.0K	./README.md
8.5M	./aws
304K	./examples
4.0K	./main.go
 16K	./scripts
 59M	./vendor
1.8M	./website

which has close to 70MB. It is unfortunate, but IMO not a huge deal and more importantly I can't think of a better solution (other than what was mentioned by @stack72 above).

bflad · 2018-01-24T15:39:06Z

Starting to take a look at this now.

At first glance through the comments above, I think I would also prefer we don't add "large" binaries to the repository unless absolutely necessary, but I have not dug into options/caveats here. Incurring additional MBs of Git download for everyone developing on the repository (versus developing the specific resource that requires it) is not friendly for folks with poor internet connections. I think in the worst case we could .gitignore the file location and t.Fatalf("Missing X file, download it from Y") or just automatically download(/build) the file when running the specific acceptance tests and its missing.

bflad

Overall this is looking pretty darn good. I left some questions/nitpicks but nothing major other than how to handle the "large" binary.

Passes for me in its current state:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSGameliftBuild'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSGameliftBuild -timeout 120m
=== RUN   TestAccAWSGameliftBuild_basic
--- PASS: TestAccAWSGameliftBuild_basic (66.44s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	66.486s

bflad · 2018-01-24T15:47:11Z

aws/resource_aws_gamelift_build.go

+				return 42, "", err
+			}
+
+			return out, *out.Build.Status, nil


Nitpick: Should we try to return a friendlier error message when the status is FAILED instead of the generic resource.StateChangeConf one for unknown status? API docs say:

FAILED -- The game build upload failed. You cannot create new fleets for this build.

Well, I'd normally opt in for providing more context if there was more context - e.g. any event feed we can pull the context (reasons of failure) from, but I think in this case the error message is more or less equivalent. It still doesn't tell you why things have failed, unfortunately.

bflad · 2018-01-24T15:53:18Z

aws/resource_aws_gamelift_build.go

+				Required:     true,
+				ValidateFunc: validation.StringLenBetween(1, 1024),
+			},
+			"operating_system": {


Nitpick: We can validate this at plan time (and maybe should since its spelled out in the docs):

func validateGameliftOperatingSystem(v interface{}, k string) (ws []string, errors []error) { value := v.(string) operatingSystems := map[string]bool{ gamelift.OperatingSystemAmazonLinux: true, gamelift.OperatingSystemWindows2012: true, } if !operatingSystems[value] { errors = append(errors, fmt.Errorf("%q must be a valid operating system value: %q", k, value)) } return }

bflad · 2018-01-24T15:58:43Z

aws/resource_aws_gamelift_build.go

+)
+
+func resourceAwsGameliftBuild() *schema.Resource {
+	return &schema.Resource{


Ugh, I see we cannot easily support imports since they do not give back the storage location information.

🤷‍♂️

bflad · 2018-01-24T16:01:38Z

aws/resource_aws_gamelift_build.go

+	_, err := conn.DeleteBuild(&gamelift.DeleteBuildInput{
+		BuildId: aws.String(d.Id()),
+	})
+	if err != nil {


Nitpick: We should ignore missing build IDs:

if err != nil { if isAwsErr(err, gamelift.ErrCodeNotFoundException, "") { return nil } return err }

We could, but DeleteBuild doesn't ever return such error if you try deleting build which doesn't exist. 🤷‍♂️

It just returns 200 OK.

bflad · 2018-01-24T16:03:40Z

aws/resource_aws_gamelift_build_test.go

+	log.Printf("[INFO] Found %d Gamelift Builds", len(resp.Builds))
+
+	for _, build := range resp.Builds {
+		if !strings.HasPrefix(*build.Name, "tf_acc_build_") {


Nitpick: I think we should use a constant here if we're going to expect continuity between the acceptance test naming and the sweeper.

Yeah, that's a good idea.

bflad · 2018-01-24T16:11:32Z

aws/resource_aws_gamelift_build_test.go

+		}
+
+		req := gamelift.DescribeBuildInput{
+			BuildId: aws.String(rs.Primary.ID),


Do we need to check rs.Primary.ID beforehand? e.g.

if rs.Primary.ID == "" { continue }

I'd say hopefully not? I mean we don't do it in in any CheckDestroy function for any other acceptance test and I'm hoping core would've already swiped the resource away from state if it doesn't have ID, if not then we have much bigger problems IMO.

bflad · 2018-01-24T16:15:23Z

aws/resource_aws_gamelift_build_test.go

+				),
+			},
+			{
+				Config: testAccAWSGameliftBuildBasicUpdateConfig(uBuildName, bucketName, zipPath, roleName, policyName),


Nitpick: I don't think we need the "UpdateConfig" since you've already parameterized the original configuration. 😄

bflad · 2018-01-24T16:19:34Z

aws/resource_aws_gamelift_build_test.go

+					resource.TestCheckResourceAttr("aws_gamelift_build.test", "operating_system", "WINDOWS_2012"),
+					resource.TestCheckResourceAttr("aws_gamelift_build.test", "storage_location.#", "1"),
+					resource.TestCheckResourceAttr("aws_gamelift_build.test", "storage_location.0.bucket", bucketName),
+					resource.TestCheckResourceAttr("aws_gamelift_build.test", "storage_location.0.key", "tf-acc-test-gl-build.zip"),


Does this need to be parameterized for parallelism/failures?

Not anymore, I found a better way which also eliminates the big ZIP file 😃

radeksimko · 2018-01-26T09:48:22Z

It took some effort but I found a way to avoid uploading the big ZIP file to the repo. 😺

I went through the Gamelift tutorial in AWS Console and traced down the location of the Amazon's sample game from CloudTrail. Then I did it for every region Gamelift is currently available in, just to make tests work in potentially any region.

bflad · 2018-01-26T11:26:13Z

aws/resource_aws_gamelift_build_test.go

+	}, nil
+}
+
+// Account ID found from CloudTrail event (role ARN) after finishing tutorial in given region


bflad

Huge shoutout for figuring out the sample apps! This very much LGTM now and let's get it in today. 👍 🚀 🎉

make testacc TEST=./aws TESTARGS='-run=TestAccAWSGameliftBuild'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSGameliftBuild -timeout 120m
=== RUN   TestAccAWSGameliftBuild_basic
--- PASS: TestAccAWSGameliftBuild_basic (18.81s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	18.862s

bflad · 2018-01-29T19:51:40Z

This has been released in terraform-provider-aws version 1.8.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-08T17:30:39Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

radeksimko added the new-resource Introduces a new resource. label Jan 3, 2018

bflad added the service/gamelift Issues and PRs that pertain to the gamelift service. label Jan 11, 2018

radeksimko requested a review from bflad January 19, 2018 19:02

bflad added this to the v1.8.0 milestone Jan 19, 2018

bflad reviewed Jan 24, 2018

View reviewed changes

radeksimko force-pushed the f-gamelift-build branch 2 times, most recently from 050ad21 to 68d6eb5 Compare January 26, 2018 09:47

New Resource: aws_gamelift_build

7c3c448

radeksimko force-pushed the f-gamelift-build branch from 68d6eb5 to 7c3c448 Compare January 26, 2018 09:53

bflad reviewed Jan 26, 2018

View reviewed changes

bflad approved these changes Jan 26, 2018

View reviewed changes

radeksimko merged commit 0c1de1f into master Jan 26, 2018

radeksimko deleted the f-gamelift-build branch January 26, 2018 12:32

ghost locked and limited conversation to collaborators Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_gamelift_build #2843

New Resource: aws_gamelift_build #2843

radeksimko commented Jan 3, 2018 •

edited

Loading

stack72 commented Jan 3, 2018

radeksimko commented Jan 3, 2018

radeksimko commented Jan 8, 2018

bflad commented Jan 24, 2018

bflad left a comment

bflad Jan 24, 2018

radeksimko Jan 26, 2018

bflad Jan 24, 2018

bflad Jan 24, 2018

radeksimko Jan 26, 2018

bflad Jan 24, 2018

radeksimko Jan 26, 2018

bflad Jan 24, 2018

radeksimko Jan 26, 2018

bflad Jan 24, 2018

radeksimko Jan 26, 2018

bflad Jan 24, 2018

bflad Jan 24, 2018

radeksimko Jan 26, 2018

radeksimko commented Jan 26, 2018

bflad Jan 26, 2018

bflad left a comment

bflad commented Jan 29, 2018

ghost commented Apr 8, 2020

New Resource: aws_gamelift_build #2843

New Resource: aws_gamelift_build #2843

Conversation

radeksimko commented Jan 3, 2018 • edited Loading

Import

Test Results

stack72 commented Jan 3, 2018

radeksimko commented Jan 3, 2018

radeksimko commented Jan 8, 2018

bflad commented Jan 24, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeksimko commented Jan 26, 2018

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

bflad commented Jan 29, 2018

ghost commented Apr 8, 2020

radeksimko commented Jan 3, 2018 •

edited

Loading