Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding servicecatalog_portfolio resource #1694

Merged
merged 15 commits into from
Oct 18, 2017
Merged

Adding servicecatalog_portfolio resource #1694

merged 15 commits into from
Oct 18, 2017

Conversation

bw-intuit
Copy link
Contributor

@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 Ninir added the new-resource Introduces a new resource. label Sep 18, 2017
Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bw-intuit

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

Thank you! 👍


d.Set("description", portfolioDetail.Description)
d.Set("display_name", portfolioDetail.DisplayName)
d.Set("id", d.Id())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

input.ProviderName = aws.String(v.(string))
}

resp, err := conn.CreatePortfolio(&input)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an instruction right before this one with:

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

?

@@ -0,0 +1,132 @@
package aws
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add the documentation please? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ninir will do

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

resp, err := conn.DescribePortfolio(&input)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

if v, ok := d.GetOk("name"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

input.ProviderName = aws.String(v.(string))
}

_, err := conn.UpdatePortfolio(&input)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

input.Id = aws.String(d.Id())

resp, err := conn.DescribePortfolio(&input)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

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

_, err := conn.DeletePortfolio(&input)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add the documentation for this resource?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ninir will do

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" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@bw-intuit bw-intuit Sep 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Thanks

@bw-intuit
Copy link
Contributor Author

bw-intuit commented Sep 19, 2017

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

@bw-intuit
Copy link
Contributor Author

@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.

screen shot 2017-09-21 at 8 32 41 am

@bw-intuit
Copy link
Contributor Author

@Ninir any feedback on the updates? Thanks

@trung
Copy link
Contributor

trung commented Oct 5, 2017

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

Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Computed: true,
ForceNew: true,
},
"name": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding the AcceptLanguage and Tags attributes?

"name": {
Type: schema.TypeString,
Required: true,
ForceNew: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"name": {
Type: schema.TypeString,
Required: true,
ForceNew: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be filled directly with:

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

}
portfolioDetail := resp.PortfolioDetail

d.Set("description", portfolioDetail.Description)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
	}

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"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const testAccCheckAwsServiceCatalogPortfolioResourceConfig_basic1 = `
resource "aws_servicecatalog_portfolio" "test" {
name = "test-1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Provides a resource to create a Service Catalog portfolio
---

# aws\_servicecatalog\_portfolio
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Service Catalog Portfolios can be imported using the `service catalog portfolio id`, e.g.

```
$ terraform import aws_servicecatalog_portfolio.testfolio port-12344321
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bw-intuit
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :) 👍

@Ninir Ninir merged commit 79826aa into hashicorp:master Oct 18, 2017
@bw-intuit
Copy link
Contributor Author

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

@Ninir
Copy link
Contributor

Ninir commented Oct 18, 2017

Awesome! ⭐️
Looking forward to this :)

@ghost
Copy link

ghost commented Apr 10, 2020

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!

@ghost ghost locked and limited conversation to collaborators Apr 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new-resource Introduces a new resource.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants