Adding servicecatalog_portfolio resource #1694

bw-intuit · 2017-09-18T16:19:42Z

@Ninir here is a pr for the first resource. Once we get this and the vendor merged I'll iterate in a similar fashion on the remaining resources.

Dependent on: #1693

More info: #1663

Ninir

Hi @bw-intuit

This looks pretty good :)
Added nitpicks which are about better debugging & documentation & cosmetic.

Thank you! 👍

Ninir · 2017-09-18T19:41:28Z

aws/resource_aws_servicecatalog_portfolio.go

+
+	d.Set("description", portfolioDetail.Description)
+	d.Set("display_name", portfolioDetail.DisplayName)
+	d.Set("id", d.Id())


This one is not needed, since SetId is already called line 75

Ninir · 2017-09-18T19:42:39Z

aws/resource_aws_servicecatalog_portfolio.go

+		input.ProviderName = aws.String(v.(string))
+	}
+
+	resp, err := conn.CreatePortfolio(&input)


Can you add an instruction right before this one with:

log.Printf("[DEBUG] Creating Service Catalog Portfolio: %#v", input)

?

Ninir · 2017-09-18T21:58:06Z

aws/resource_aws_servicecatalog_portfolio.go

@@ -0,0 +1,132 @@
+package aws


Can you also add the documentation please? :)

@Ninir will do

Ninir · 2017-09-18T21:58:59Z

aws/resource_aws_servicecatalog_portfolio.go

+	input := servicecatalog.DescribePortfolioInput{}
+	input.Id = aws.String(d.Id())
+
+	resp, err := conn.DescribePortfolio(&input)


Would be nice to have a log instruction saying that we are actually reading the portfolio, for better debugging!

Ninir · 2017-09-18T22:04:23Z

aws/resource_aws_servicecatalog_portfolio.go

+	input := servicecatalog.UpdatePortfolioInput{}
+	input.Id = aws.String(d.Id())
+
+	if v, ok := d.GetOk("name"); ok {


d.GetOk will always returns the value plus the existence check if the field is set.
We better need d.HasChange("name") here and below, so that Terraform is well-aware of changes and then use GetOk to get the value.

This way, we only update fields that we need to update, and reduce the amount of edge cases. Thoughts?

Ninir · 2017-09-18T22:04:44Z

aws/resource_aws_servicecatalog_portfolio.go

+		input.ProviderName = aws.String(v.(string))
+	}
+
+	_, err := conn.UpdatePortfolio(&input)


Here also, would be nice to have a log instruction with parameters, as for the create method

Ninir · 2017-09-18T22:06:44Z

aws/resource_aws_servicecatalog_portfolio.go

+	input.Id = aws.String(d.Id())
+
+	resp, err := conn.DescribePortfolio(&input)
+	if err != nil {


We should really check for ResourceNotFoundException and if this is the case (removed from the console for instance), we should add a log instruction like:

log.Printf("[WARN] Service Catalog %q not found, removing from state", d.Id())

Ninir · 2017-09-18T22:07:11Z

aws/resource_aws_servicecatalog_portfolio.go

+	input := servicecatalog.DeletePortfolioInput{}
+	input.Id = aws.String(d.Id())
+
+	_, err := conn.DeletePortfolio(&input)


Here also, a log instruction saying that we are deleting would be nice :)

Ninir · 2017-09-18T22:07:28Z

aws/provider.go

@@ -438,6 +438,7 @@ func Provider() terraform.ResourceProvider {
 			"aws_network_interface_sg_attachment":          resourceAwsNetworkInterfaceSGAttachment(),
 			"aws_default_security_group":                   resourceAwsDefaultSecurityGroup(),
 			"aws_security_group_rule":                      resourceAwsSecurityGroupRule(),
+			"aws_servicecatalog_portfolio":                 resourceAwsServiceCatalogPortfolio(),


Can you also add the documentation for this resource?

@Ninir will do

bw-intuit · 2017-09-19T01:37:28Z

aws/resource_aws_servicecatalog_portfolio.go

+	log.Printf("[DEBUG] Reading Service Catalog Portfolio: %#v", input)
+	resp, err := conn.DescribePortfolio(&input)
+	if err != nil {
+		if scErr, ok := err.(awserr.Error); ok && scErr.Code() == "ResourceNotFoundException" {


@Ninir can you suggest, or point me to a similar example, how to idiomatically write a test for this type of error in the tf test framework?

This kind of test is not simple to implement, due to the way to test: we need to create the resource, add a custom step to remove the object, and expect a non-empty plan.

In this example, you first have a test that expects an error, and then a test that expects a non empty plan.
Depending on our implementation, we will go for one or the other (I think it will be the non-empty plan)

The best way to proceed is perhaps to make build the provider and check it manually.

@Ninir Ran it manually, removed the resource and it works.

2017/09/19 06:17:46 [WARN] Service Catalog "port-123456781234" not found, removing from state

Still a bit hazy but will try to construct a non-empty plan test using the above.

@Ninir added a first pass at a test. Can you provide feedback?

Thanks

bw-intuit · 2017-09-19T13:04:07Z

@Ninir added a first pass at documentation. Is there a way I can run the site to test myself?

bw-intuit · 2017-09-21T15:34:39Z

@Ninir added docs and tested. Believe I have addressed all your current requests and updates to the tests. Can you provide some feedback? I have time this week and would like to hammer out the rest of them if this is moving in the right direction.

bw-intuit · 2017-09-27T20:44:01Z

@Ninir any feedback on the updates? Thanks

trung · 2017-10-05T19:39:14Z

I would like to give hands on this. How should we proceed?

Ninir

Hey @bw-intuit

This looks very great! Just left a few more comments to address and handle, and it should be good to merge!

Thanks a lot for this :)

Ninir · 2017-10-10T20:26:51Z

aws/resource_aws_servicecatalog_portfolio.go

+				Computed: true,
+				ForceNew: true,
+			},
+			"name": {


What about adding the AcceptLanguage and Tags attributes?

Ninir · 2017-10-10T20:29:19Z

aws/resource_aws_servicecatalog_portfolio.go

+			"name": {
+				Type:     schema.TypeString,
+				Required: true,
+				ForceNew: false,


ForceNew being false is the default value, so no need to specify it :)

Ninir · 2017-10-10T20:30:49Z

aws/resource_aws_servicecatalog_portfolio.go

+			"name": {
+				Type:     schema.TypeString,
+				Required: true,
+				ForceNew: false,


We should also validate each atttribute, as in: http://docs.aws.amazon.com/servicecatalog/latest/dg/API_CreatePortfolio.html#servicecatalog-CreatePortfolio-request-ProviderName.
For this one, name should be required and have a max length of 20 for instance

Ninir · 2017-10-10T20:32:16Z

aws/resource_aws_servicecatalog_portfolio.go

+func resourceAwsServiceCatalogPortfolioCreate(d *schema.ResourceData, meta interface{}) error {
+	conn := meta.(*AWSClient).scconn
+	input := servicecatalog.CreatePortfolioInput{}
+	if v, ok := d.GetOk("name"); ok {


When an attribute is required, GetOk will return false, so we can directly fill the input knowing that

Ninir · 2017-10-10T20:33:58Z

aws/resource_aws_servicecatalog_portfolio.go

+func resourceAwsServiceCatalogPortfolioUpdate(d *schema.ResourceData, meta interface{}) error {
+	conn := meta.(*AWSClient).scconn
+	input := servicecatalog.UpdatePortfolioInput{}
+	input.Id = aws.String(d.Id())


This could be filled directly with:

input := servicecatalog.UpdatePortfolioInput{ Id: aws.String(d.Id()), }

Ninir · 2017-10-10T20:42:24Z

aws/resource_aws_servicecatalog_portfolio.go

+	}
+	portfolioDetail := resp.PortfolioDetail
+
+	d.Set("description", portfolioDetail.Description)


We should also have those attributes: arn (computed), created_time (computed)

Created time should be set as in:

if err := d.Set("created_time", portfolioDetail.CreatedTime.Format(time.RFC3339)); err != nil { log.Printf("[DEBUG] Error setting created_time: %s", err) }

Ninir · 2017-10-10T20:43:18Z

aws/resource_aws_servicecatalog_portfolio_test.go

+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("aws_servicecatalog_portfolio.test", "name", "test-1"),
+					resource.TestCheckResourceAttr("aws_servicecatalog_portfolio.test", "description", "test-2"),
+					resource.TestCheckResourceAttr("aws_servicecatalog_portfolio.test", "provider_name", "test-3"),


When we'll have more attributes, we should also check them here :)

Ninir · 2017-10-10T20:46:19Z

aws/resource_aws_servicecatalog_portfolio_test.go

+
+const testAccCheckAwsServiceCatalogPortfolioResourceConfig_basic1 = `
+resource "aws_servicecatalog_portfolio" "test" {
+  name = "test-1"


We should definitely randomize test values, so that we don't have conflicts when running nightly tests.
FOr instance, we can pass a given value, and then use it

Ninir · 2017-10-10T20:46:53Z

website/docs/r/servicecatalog_portfolio.html.markdown

+  Provides a resource to create a Service Catalog portfolio
+---
+
+# aws\_servicecatalog\_portfolio


Those antislashes aren't needed anymore due to a recent work by Seth Vargo :)

Ninir · 2017-10-10T20:49:22Z

website/docs/r/servicecatalog_portfolio.html.markdown

+Service Catalog Portfolios can be imported using the `service catalog portfolio id`, e.g.
+
+```
+$ terraform import aws_servicecatalog_portfolio.testfolio port-12344321


Can you also add a test for imports? as in: https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/import_aws_vpc_test.go

bw-intuit · 2017-10-15T17:06:16Z

@Ninir updated based on code review. AcceptLanguage is a request parameter which is not persisted. I've hard set it to "en" for now. How you would recommend parameters such as this are processed?

bw-intuit · 2017-10-16T15:27:06Z

@Ninir with the exception of my question around AcceptLanguage, all other comments should be addressed. Can you review and provide feedback? Thanks

Ninir

Hi @bw-intuit

Just ran the tests and everything is green:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSServiceCatalogPortfolio'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSServiceCatalogPortfolio -timeout 120m
=== RUN   TestAccAWSServiceCatalogPortfolioBasic
--- PASS: TestAccAWSServiceCatalogPortfolioBasic (32.79s)
=== RUN   TestAccAWSServiceCatalogPortfolioDisappears
--- PASS: TestAccAWSServiceCatalogPortfolioDisappears (8.75s)
=== RUN   TestAccAWSServiceCatalogPortfolioImport
--- PASS: TestAccAWSServiceCatalogPortfolioImport (13.92s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	55.503s

Since there was a conflict in validators.go, took th liberty to rebase & update the work.

I would like to thank you a lot for the work, and all the efforts you put there, we really appreciate your reactivity and engagement :) 👍

bw-intuit · 2017-10-18T21:22:41Z

@Ninir thanks for the merge! I will submit PRs for the remaining resources for service catalog using the same approach.

Ninir · 2017-10-18T21:34:56Z

Awesome! ⭐️
Looking forward to this :)

ghost · 2020-04-10T17:52: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 mentioned this pull request Sep 18, 2017

vendor updates for servicecatalog #1693

Merged

Ninir added the new-resource Introduces a new resource. label Sep 18, 2017

Ninir reviewed Sep 18, 2017

View reviewed changes

bw-intuit commented Sep 19, 2017

View reviewed changes

Ninir reviewed Oct 10, 2017

View reviewed changes

bw-intuit added 15 commits October 18, 2017 22:57

adding servicecatalog portfolio resource

4a84730

updates per code review

9c9c08b

adding documentation for service catalog portfolio

7fc04c0

fixing documentation names for service catalog

4b1d254

update service catalog documentation

04bf0d2

adding destroy and dissapear test

8226851

update loggin messages for consistency

485e4e1

update sidebar

0bec902

updates based on code review

79bb936

adding support for tags

054678b

adding tests for tags

10c9338

formatting

c35df43

update name

5721ae9

updated to consistent names

7b6fe33

adding created time and arn

c20b0ec

Ninir approved these changes Oct 18, 2017

View reviewed changes

Ninir merged commit 79826aa into hashicorp:master Oct 18, 2017

ghost locked and limited conversation to collaborators Apr 10, 2020

Adding servicecatalog_portfolio resource #1694

Adding servicecatalog_portfolio resource #1694

Conversation

bw-intuit commented Sep 18, 2017

Ninir 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

bw-intuit Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

bw-intuit commented Sep 19, 2017 • edited Loading

bw-intuit commented Sep 21, 2017

bw-intuit commented Sep 27, 2017

trung commented Oct 5, 2017

Ninir 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

bw-intuit commented Oct 15, 2017

bw-intuit commented Oct 16, 2017

Ninir left a comment

Choose a reason for hiding this comment

bw-intuit commented Oct 18, 2017

Ninir commented Oct 18, 2017

ghost commented Apr 10, 2020

bw-intuit Sep 19, 2017 •

edited

Loading

bw-intuit commented Sep 19, 2017 •

edited

Loading