New Resource: aws_servicecatalog_product #2064

bw-intuit · 2017-10-26T14:20:13Z

@Ninir working on the service catalog product resource and wanted to get feedback on some coding decisions.

Each SC product will need to be created with an artifact (http://docs.aws.amazon.com/servicecatalog/latest/dg/API_CreateProduct.html#servicecatalog-CreateProduct-request-ProvisioningArtifactParameters)

I have moved the variables that describe it to an artifact block which will be managed as part of the service catalog resource for example:

resource "aws_servicecatalog_product" "test" {
  description = "doc"
  distributor = "dist"
  name = "nm"
  owner = "own"
  product_type = "CLOUD_FORMATION_TEMPLATE"
  support_description = "doc"
  support_email = "u@e.com"
  support_url = "support_url"

  artifact {
    description = "ad"
    name = "an"
    load_template_from_url = "https://s3-${var.region}.amazonaws.com/${aws_s3_bucket.bucket.id}/${aws_s3_bucket_object.template.key}"
    type = "CLOUD_FORMATION_TEMPLATE"
  }
}

There will also be a resource created for additional artifacts, however it will not be able to manage the artifact created with the product (http://docs.aws.amazon.com/servicecatalog/latest/dg/API_CreateProvisioningArtifact.html).

Does this approach make sense? Let me know and I'll polish it for review.

bw-intuit · 2017-11-02T15:28:06Z

@Ninir can you do a quick review and provide feedback on the suggested approach when you have a moment?

radeksimko

Hi @bw-intuit
thanks for sending the PR.

I left you some comments there. It may feel like a lot of feedback, but I'm confident we'll get through it together 😅 Let me know if anything's unclear or if you need further help.

Besides what I mentioned inline:

It seems that the resource creation can take a short while and we should wait until the creation is complete - i.e. use StateChangeConf, example here: https://github.com/terraform-providers/terraform-provider-aws/blob/680afc8f836c14074d5502dc820128d289f552f3/aws/resource_aws_batch_job_queue.go#L65-L77
Can you add a test which is adding, updating and removing artifacts?
Do you mind adding documentation for this new resource?

radeksimko · 2018-04-06T10:49:00Z

aws/resource_aws_servicecatalog_product.go

+
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/awserr"
+


Nitpick: We tend to group all 3rd party imports in a single block.

radeksimko · 2018-04-06T10:49:35Z

aws/resource_aws_servicecatalog_product.go

+			Create: schema.DefaultTimeout(30 * time.Minute),
+			Update: schema.DefaultTimeout(30 * time.Minute),
+			Delete: schema.DefaultTimeout(30 * time.Minute),
+		},


Those custom timeout are not used anywhere, do you mind removing the definition?

good catch, removing

radeksimko · 2018-04-06T10:50:27Z

aws/resource_aws_servicecatalog_product.go

+		},
+
+		Schema: map[string]*schema.Schema{
+			"created_time": {


Nitpick: Is there any good use case / reason for exposing this field? I'm not suggesting it's necessarily wrong, just curious what the user would do with it.

No specific use case, removing. I didn't know the philosophy on including lesser used field for completeness vs being a bit more concise.

radeksimko · 2018-04-06T10:51:47Z

aws/resource_aws_servicecatalog_product.go

+				Type:     schema.TypeString,
+				Required: true,
+			},
+			"id": {


Computed id (meta)field is always automatically available for all resources which is why we disallow the explicit definition in the schema:

TF_ACC=1 go test ./aws -v -run=TestAccAWSServiceCatalogProduct -timeout 120m === RUN TestAccAWSServiceCatalogProductBasic --- FAIL: TestAccAWSServiceCatalogProductBasic (3.49s) testing.go:518: Step 0 error: config is invalid: provider.aws: Internal validation of the provider failed! This is always a bug with the provider itself, and not a user issue. Please report this bug: 1 error(s) occurred: * resource aws_servicecatalog_product: id is a reserved field name

Do you mind removing it?

Got it, removing

radeksimko · 2018-04-06T11:09:50Z

aws/resource_aws_servicecatalog_product.go

+	conn := meta.(*AWSClient).scconn
+	input := servicecatalog.CreateProductInput{}
+
+	if v, ok := d.GetOk("name"); ok {


Generally speaking GetOk helper is there for checking if an optional field was defined or not. This field is however required (as well as many other fields below which are being checked via GetOk). Using Get() would also allow us to simplify the code here a bit, e.g.

input := servicecatalog.CreateProductInput{ IdempotencyToken: resource.UniqueId(), Name: aws.String(d.Get("name").(string)), Owner: aws.String(d.Get("owner").(string)), ProductType: aws.String(d.Get("product_type").(string)), ProvisioningArtifactParameters: expandServiceCatalogProvisioningArtifacts(d.Get("provisioning_artifact").(*schema.Set).List()) }

which brings me to two other notes:

We should generate a token we know for sure is going to be unique regardless of time. The chance of generating the same token twice is low, but not entirely impossible as Terraform can create resources in parallel. The helper I used in the above snippet comes from github.com/hashicorp/terraform/helper/resource and we use it for this exact purpose in other resources, e.g. EFS. While the implementation looks very similar (uses time too) it also uses mutex to guarantee uniqueness, at least within the scope of a single process.

We tend to decouple all logic that has anything to do with conversation between schema-friendly format (map[string]interface{}/schema.Set etc.) and SDK structs. We call these functions expanders (schema -> SDK struct) and flatteners (SDK struct -> schema). This allows us to simplify and shorten overall LOC in the CRUD and make it easier to read. We usually keep all those helpers in aws/structure.go.

radeksimko · 2018-04-06T15:20:45Z

aws/resource_aws_servicecatalog_product_test.go

+	"github.com/hashicorp/terraform/terraform"
+)
+
+func TestAccAWSServiceCatalogProductBasic(t *testing.T) {


Do you mind following the convention of TestAccAWSResourceName_(basic|another|test) here? This convention allows us to easily filter tests for a specific resource without unintentionally running tests for other resources whose name overlaps with another resource.

e.g. if Amazon decided to introduce ServiceCatalogProductSomething then we'd be forced to run tests for that resource because the only filter we can use is -run=TestAccAWSServiceCatalogProduct. If instead we add the underscore we can filter like -run=TestAccAWSServiceCatalogProduct_ or -run=TestAccAWSServiceCatalogProductSomething_ which is much more precise.

radeksimko · 2018-04-06T15:21:23Z

aws/resource_aws_servicecatalog_product_test.go

+	}
+}
+
+func testAccCheckServiceCatlaogProductDestroy(s *terraform.State) error {


Typo ^ 👀

radeksimko · 2018-04-06T15:21:48Z

aws/resource_aws_servicecatalog_product_test.go

+	conn := testAccProvider.Meta().(*AWSClient).scconn
+
+	for _, rs := range s.RootModule().Resources {
+		if rs.Type != "aws_servicecatalog_portfolio" {


Copy-paste error? I assume this should be aws_servicecatalog_product.

radeksimko · 2018-04-06T15:24:19Z

aws/resource_aws_servicecatalog_product_test.go

+		_, err := conn.DescribeProduct(&input)
+		if err == nil {
+			return fmt.Errorf("Product still exists")
+		}


Do you mind filtering the error here via isAWSErr(err, servicecatalog.ErrCodeResourceNotFoundException, "") and still error out on any other error? i.e.

if err != nil { if isAWSErr(err, servicecatalog.ErrCodeResourceNotFoundException, "") { return nil } return err } return fmt.Errorf("Product still exists")

radeksimko · 2018-04-06T16:19:11Z

aws/resource_aws_servicecatalog_product.go

+									return true
+								}
+								return false
+							},


We can in fact source the URL from the API, so we shouldn't need the suppression function here:
https://docs.aws.amazon.com/servicecatalog/latest/dg/API_DescribeProvisioningArtifact.html#servicecatalog-DescribeProvisioningArtifact-response-Info

bw-intuit · 2018-05-03T17:59:13Z

@radeksimko was about to get create / read / import working. Can you verify the direction is accurate and I will cleanup the code, address remaining comments and implement remaining functions? Thanks in advance

trung · 2018-05-15T18:07:28Z

Hope this PR will be done soon so i can enrich my tests for service catalog related data sources (product, launch path and provisioning artifact) and resource (product provisioning), making it ready for PR

trung · 2018-06-08T21:23:40Z

@bw-intuit i can give a hand on this if you don't mind.

trung · 2018-06-21T17:35:51Z

I'm forking @bw-intuit branch and making some changes per @radeksimko comments. I would likely to create a new PR to facilitate the review. Any objection?

trung · 2018-06-25T19:47:38Z

I created PR #4980 for review. I took into consideration all review comments and it's based on @bw-intuit branch.

bflad · 2019-08-20T03:49:46Z

This appears to be superseded by #4980, which contains all the previous commits, so closing this one in preference of the newer pull request.

ghost · 2019-11-01T15:31:31Z

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!

bw-intuit added 2 commits October 25, 2017 11:56

refactoring artifact definition

b7a391d

wip adding artifact block

b157049

radeksimko added the new-resource Introduces a new resource. label Oct 26, 2017

bw-intuit added 2 commits November 12, 2017 17:26

refactoring to support new format

acaf0fa

minor code cleanup

26b4bee

radeksimko added the size/XL Managed by automation to categorize the size of a PR. label Nov 15, 2017

radeksimko added the service/servicecatalog Issues and PRs that pertain to the servicecatalog service. label Jan 16, 2018

radeksimko changed the title ~~Adding Service Catalog product~~ New Resource: aws_servicecatalog_product Jan 16, 2018

radeksimko self-requested a review April 5, 2018 17:04

radeksimko suggested changes Apr 6, 2018

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Apr 6, 2018

Merge branch 'master' into service_catalog_product

6cba05a

ghost added the size/XL Managed by automation to categorize the size of a PR. label Apr 9, 2018

bw-intuit added 2 commits April 9, 2018 15:04

update based on code review

e2f82d8

updates based on code review

b2fcd7c

ghost added the size/XL Managed by automation to categorize the size of a PR. label Apr 10, 2018

refactoring to updated template structure

700d0e6

ghost added the size/XL Managed by automation to categorize the size of a PR. label May 3, 2018

bw-intuit added 2 commits May 3, 2018 10:24

fix test

0ae86b9

minor code / test cleanup

f5548f0

ghost added the size/XL Managed by automation to categorize the size of a PR. label May 3, 2018

bw-intuit added 2 commits May 3, 2018 10:36

adding comment

dc83bab

reading template url, fix import

9d4bf7b

trung mentioned this pull request Jun 25, 2018

New Resource: aws_servicecatalog_product (compliment PR 2064) #4980

Closed

bw-intuit mentioned this pull request Nov 1, 2018

Add Support For AWS Service Catalog #1663

Closed

bflad closed this Aug 20, 2019

ghost locked and limited conversation to collaborators Nov 1, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Nov 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_servicecatalog_product #2064

New Resource: aws_servicecatalog_product #2064

bw-intuit commented Oct 26, 2017

bw-intuit commented Nov 2, 2017

radeksimko left a comment •

edited

Loading

radeksimko Apr 6, 2018

bw-intuit Apr 9, 2018

radeksimko Apr 6, 2018

bw-intuit Apr 9, 2018

radeksimko Apr 6, 2018

bw-intuit Apr 9, 2018

radeksimko Apr 6, 2018

bw-intuit Apr 9, 2018

radeksimko Apr 6, 2018

radeksimko Apr 6, 2018

radeksimko Apr 6, 2018

radeksimko Apr 6, 2018

radeksimko Apr 6, 2018

radeksimko Apr 6, 2018

bw-intuit commented May 3, 2018

trung commented May 15, 2018

trung commented Jun 8, 2018

trung commented Jun 21, 2018

trung commented Jun 25, 2018

bflad commented Aug 20, 2019

ghost commented Nov 1, 2019


		"github.com/aws/aws-sdk-go/aws"
		"github.com/aws/aws-sdk-go/aws/awserr"

New Resource: aws_servicecatalog_product #2064

New Resource: aws_servicecatalog_product #2064

Conversation

bw-intuit commented Oct 26, 2017

bw-intuit commented Nov 2, 2017

radeksimko left a comment • edited Loading

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

bw-intuit commented May 3, 2018

trung commented May 15, 2018

trung commented Jun 8, 2018

trung commented Jun 21, 2018

trung commented Jun 25, 2018

bflad commented Aug 20, 2019

ghost commented Nov 1, 2019

radeksimko left a comment •

edited

Loading