r/servicecatalog_portfolio_share: New resource(s) #19278

YakDriver · 2021-05-07T17:02:18Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates #15369
Relates #18074
Relates #19122

Right now I'm having trouble testing this since at least the organizations access requires root, non-org account access, which I don't have. Also, these resources are unlikely to be the heaviest used and perhaps relying on the community with these would be okay. 🤷

Output from acceptance testing (us-west-2):

--- SKIP: TestAccAWSServiceCatalogPortfolioShare_organization (0.87s)
--- PASS: TestAccAWSServiceCatalogPortfolioShare_basic (17.98s)

--- SKIP: TestAccAWSServiceCatalogOrganizationsAccess_basic (1.00s)

Output from acceptance testing (GovCloud):

--- SKIP: TestAccAWSServiceCatalogPortfolioShare_basic (1.28s)
--- SKIP: TestAccAWSServiceCatalogPortfolioShare_organization (1.79s)

--- SKIP: TestAccAWSServiceCatalogOrganizationsAccess_basic (1.00s)

Note: I'm not sure an AWS Organizations tests can ever run in GovCloud. GovCloud accounts are always tied to an existing commercial account so may not have the capacity of being a "payer" account.

ewbankkit · 2021-05-10T19:57:34Z

aws/resource_aws_servicecatalog_organizations_access.go

+		return fmt.Errorf("error enabling Service Catalog AWS Organizations Access: %w", err)
+	}
+
+	d.SetId(meta.(*AWSClient).accountid)


ID not set in the enabled branch above.

Nice catch!!

ewbankkit · 2021-05-10T19:57:55Z

aws/resource_aws_servicecatalog_organizations_access.go

+	_, err := conn.DisableAWSOrganizationsAccess(&servicecatalog.DisableAWSOrganizationsAccessInput{})
+
+	if err != nil {
+		return fmt.Errorf("error enabling Service Catalog AWS Organizations Access: %w", err)


Suggested change

return fmt.Errorf("error enabling Service Catalog AWS Organizations Access: %w", err)

return fmt.Errorf("error disabling Service Catalog AWS Organizations Access: %w", err)

ewbankkit · 2021-05-10T20:35:59Z

aws/resource_aws_servicecatalog_organizations_access.go

+			return fmt.Errorf("error disabling Service Catalog AWS Organizations Access: %w", err)
+		}
+
+		return resourceAwsServiceCatalogOrganizationsAccessRead(d, meta)


It's not a standard pattern (that I've ever seen) to call Read from Delete, usually just return nil.

Copy pasta! 🤦

ewbankkit · 2021-05-10T20:36:30Z

aws/resource_aws_servicecatalog_organizations_access.go

+		return fmt.Errorf("error disabling Service Catalog AWS Organizations Access: %w", err)
+	}
+
+	return resourceAwsServiceCatalogOrganizationsAccessRead(d, meta)


Blerg. 🤦

ewbankkit · 2021-05-10T20:39:38Z

aws/resource_aws_servicecatalog_portfolio.go

-				Required:     true,
-				ValidateFunc: validation.StringLenBetween(1, 20),
+				Type:     schema.TypeString,
+				Required: true,


Do we want to enforce documented max length of 100?

I have mixed feelings about enforcing these - mostly against. AWS changes them (across the API) regularly and so we're enforcing the wrong limits much of the time. But, it may save a long wait in certain situations. Most of the time though, AWS responds quickly if validation is failing.

ewbankkit · 2021-05-10T20:40:11Z

aws/resource_aws_servicecatalog_portfolio.go

-				Optional:     true,
-				ValidateFunc: validation.StringLenBetween(1, 20),
+				Type:     schema.TypeString,
+				Optional: true,


Do we want to enforce documented max length of 50?

ewbankkit · 2021-05-10T20:46:01Z

aws/resource_aws_servicecatalog_portfolio_share.go

+		return fmt.Errorf("error creating Service Catalog Portfolio Share: empty response")
+	}
+
+	d.SetId(strings.Join([]string{d.Get("portfolio_id").(string), d.Get("type").(string), d.Get("principal_id").(string)}, ":"))


Bonus points: Move to aws/internal/service/servicecatalog/id.go.

ewbankkit · 2021-05-10T20:46:14Z

aws/resource_aws_servicecatalog_portfolio_share.go

+	return nil
+}
+
+func resourceServiceCatalogPortfolioShareParseId(id string) (string, string, string, error) {


Bonus points: Move to aws/internal/service/servicecatalog/id.go.

ewbankkit · 2021-05-10T20:51:44Z

Verified acceptance tests in Commercial partition:

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSServiceCatalogPortfolioShare_\|TestAccAWSServiceCatalogOrganizationsAccess_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSServiceCatalogPortfolioShare_\|TestAccAWSServiceCatalogOrganizationsAccess_ -timeout 180m
=== RUN   TestAccAWSServiceCatalogOrganizationsAccess_basic
=== PAUSE TestAccAWSServiceCatalogOrganizationsAccess_basic
=== RUN   TestAccAWSServiceCatalogPortfolioShare_basic
=== PAUSE TestAccAWSServiceCatalogPortfolioShare_basic
=== RUN   TestAccAWSServiceCatalogPortfolioShare_organization
=== PAUSE TestAccAWSServiceCatalogPortfolioShare_organization
=== CONT  TestAccAWSServiceCatalogOrganizationsAccess_basic
=== CONT  TestAccAWSServiceCatalogPortfolioShare_organization
=== CONT  TestAccAWSServiceCatalogPortfolioShare_basic
=== CONT  TestAccAWSServiceCatalogOrganizationsAccess_basic
    provider_test.go:780: skipping tests; this AWS account must not be an existing member of an AWS Organization
--- SKIP: TestAccAWSServiceCatalogOrganizationsAccess_basic (0.98s)
=== CONT  TestAccAWSServiceCatalogPortfolioShare_organization
    provider_test.go:780: skipping tests; this AWS account must not be an existing member of an AWS Organization
--- SKIP: TestAccAWSServiceCatalogPortfolioShare_organization (0.98s)
--- PASS: TestAccAWSServiceCatalogPortfolioShare_basic (19.02s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	21.943s

…ion management account. Acceptance test output: % make testacc TEST=./aws TESTARGS='-run=TestAccAWSServiceCatalogOrganizationsAccess_' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSServiceCatalogOrganizationsAccess_ -timeout 180m === RUN TestAccAWSServiceCatalogOrganizationsAccess_basic === PAUSE TestAccAWSServiceCatalogOrganizationsAccess_basic === CONT TestAccAWSServiceCatalogOrganizationsAccess_basic --- PASS: TestAccAWSServiceCatalogOrganizationsAccess_basic (16.23s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 19.053s % make testacc TEST=./aws TESTARGS='-run=TestAccAWSServiceCatalogPortfolioShare_' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSServiceCatalogPortfolioShare_ -timeout 180m === RUN TestAccAWSServiceCatalogPortfolioShare_basic === PAUSE TestAccAWSServiceCatalogPortfolioShare_basic === RUN TestAccAWSServiceCatalogPortfolioShare_organizationalUnit === PAUSE TestAccAWSServiceCatalogPortfolioShare_organizationalUnit === CONT TestAccAWSServiceCatalogPortfolioShare_basic === CONT TestAccAWSServiceCatalogPortfolioShare_organizationalUnit --- PASS: TestAccAWSServiceCatalogPortfolioShare_basic (19.43s) --- PASS: TestAccAWSServiceCatalogPortfolioShare_organizationalUnit (24.29s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 27.222s

ghost · 2021-05-19T01:07:08Z

This has been released in version 3.41.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

github-actions · 2021-06-18T02:14:19Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

YakDriver requested a review from a team as a code owner May 7, 2021 17:02

ewbankkit self-assigned this May 10, 2021

ewbankkit reviewed May 10, 2021

View reviewed changes

YakDriver force-pushed the f-servicecat-portfolio-share branch from ab97a3f to e338096 Compare May 11, 2021 17:45

YakDriver requested a review from ewbankkit May 11, 2021 17:53

YakDriver and others added 21 commits May 14, 2021 13:22

docs/r/servicecatalog_portfolio_share: Update for schema rework

14d340f

docs/r/servicecatalog_portfolio_share: Add wait arg

92253f3

internal/servicecatalog: Adjust not found errors

5d259d7

internal/servicecatalog: Adjust not found errors

a4ea865

tests/r/servicecatalog_portfolio_share: Add new account test

ea82747

r/servicecatalog_portfolio_share: Adjust for wait option

75e28a7

envvar: Add skipping option func

547f518

provider: Skip when prequisite alt acct envvars not set

34ccacb

tests/r/servicecatalog_portfolio_share: Add prechecks

c1a29fd

tests/r/servicecatalog_portfolio_share: Hardcoded fix

a1bfca5

envvar: Update skip func docs

44861f3

provider: Add servicecatalog resource

56e278b

tests/r/servicecatalog_portfolio_share: Remove hardcoded partition

cd74e57

r/servicecatalog_portfolio_share: Move id stuff to internal

a6166f5

r/servicecat_org_access: Clean up messages, delete

26c06ed

r/servicecat_portfolio: Enforce field lengths

6d12cff

r/servicecat_portfolio_share: Use internal ID helper

ea01f5c

Add 'testAccOrganizationManagementAccountPreCheck'.

1eb17f2

Fix type casting error.

40d9bbe

r/servicecat_portfolio_share: Revise org test

0b71dd4

r/servicecat_portfolio_share: Finder contains string

cb8d633

YakDriver force-pushed the f-servicecat-portfolio-share branch from 08cd931 to cb8d633 Compare May 14, 2021 17:22

YakDriver added this to the v3.41.0 milestone May 14, 2021

YakDriver merged commit 27a5052 into main May 14, 2021

YakDriver deleted the f-servicecat-portfolio-share branch May 14, 2021 18:53

github-actions bot pushed a commit that referenced this pull request May 14, 2021

Update CHANGELOG.md for #19278

2c35f4c

github-actions bot locked as resolved and limited conversation to collaborators Jun 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/servicecatalog_portfolio_share: New resource(s) #19278

r/servicecatalog_portfolio_share: New resource(s) #19278

YakDriver commented May 7, 2021 •

edited

Loading

ewbankkit May 10, 2021

YakDriver May 11, 2021

ewbankkit May 10, 2021

YakDriver May 11, 2021

ewbankkit May 10, 2021

YakDriver May 11, 2021

ewbankkit May 10, 2021

YakDriver May 11, 2021

ewbankkit May 10, 2021

YakDriver May 11, 2021

ewbankkit May 10, 2021

YakDriver May 11, 2021

ewbankkit May 10, 2021

YakDriver May 11, 2021

ewbankkit May 10, 2021

YakDriver May 11, 2021

ewbankkit commented May 10, 2021

ghost commented May 19, 2021

github-actions bot commented Jun 18, 2021

	return fmt.Errorf("error enabling Service Catalog AWS Organizations Access: %w", err)
	return fmt.Errorf("error disabling Service Catalog AWS Organizations Access: %w", err)

r/servicecatalog_portfolio_share: New resource(s) #19278

r/servicecatalog_portfolio_share: New resource(s) #19278

Conversation

YakDriver commented May 7, 2021 • edited Loading

Community Note

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

Choose a reason for hiding this comment

ewbankkit commented May 10, 2021

ghost commented May 19, 2021

github-actions bot commented Jun 18, 2021

YakDriver commented May 7, 2021 •

edited

Loading