Service Catalog Product resource added #13834
Conversation
…t-and-pp fix trivial conflicts where master has changed how resources are listed (in website and in aws/provider), to reference servicecatalog_product in the right way also revert the mode change against the servicecatalog_portfolio markdown
only pulling in the relevant provisioned-product changes
* create needs to block for completion, then the test (as written) will work in our account * then we need product_portfolio_association and make the test work in any account (create a product, not hard-coded) * then other TODOs marked in code
* master: (79 commits) Update CHANGELOG for hashicorp#8457 resource/aws_ec2_tag: Finish implementation CodeArtifact new service support (hashicorp#13735) internal/keyvaluetags: Support servicediscovery and worklink (hashicorp#13732) tests/provider: Remove TravisCI (hashicorp#13730) update method params in web_acl test check Update CHANGELOG for hashicorp#12688 Fix rebase change Fix docs Improve error message Apply review comments Refactor after rebase Apply review comments Apply changes based on review Format test config Fix failing build Add calculated wcus attribute Add documentation Add rule_group_reference_statement Add rate_based_statement ...
Match the same method used in other places within the provider to create idempotency tokens.
|
This has been updated as per the 18 Aug notification to use the v2 sdk and remove |
| Type: schema.TypeString, | ||
| Required: true, | ||
| ForceNew: true, | ||
| ValidateFunc: validation.StringInSlice([]string{ |
There was a problem hiding this comment.
now that enum slices exist lets change this.
ValidateFunc: validation.StringInSlice(servicecatalog.ProductType_Values(), false),
| Optional: true, | ||
| ForceNew: true, | ||
| Default: servicecatalog.ProvisioningArtifactTypeCloudFormationTemplate, | ||
| ValidateFunc: validation.StringInSlice([]string{ |
There was a problem hiding this comment.
enums slice same as above
|
|
||
| func resourceAwsServiceCatalogProductCreate(d *schema.ResourceData, meta interface{}) error { | ||
| conn := meta.(*AWSClient).scconn | ||
| input := servicecatalog.CreateProductInput{} |
There was a problem hiding this comment.
can move all required argument inline and use Get instead of GetOk for them.
| return fmt.Errorf("setting ProvisioningArtifact for product '%s' failed: %s", d.Id(), err) | ||
| } | ||
|
|
||
| // TODO budgets |
There was a problem hiding this comment.
lets remove these comments(open a separate issue for it) or add these as computed
| v, _ := d.GetOk("description") | ||
| input.Description = aws.String(v.(string)) |
There was a problem hiding this comment.
input.Description = aws.String(d.Get("description").(string))
relevant for all the update uses later as well
| } | ||
|
|
||
| // figure out what tags to add and what tags to remove | ||
| if d.HasChange("tags") { |
There was a problem hiding this comment.
if d.HasChange("tags") {
o, n := d.GetChange("tags")
input.AddTags = keyvaluetags.New(n).IgnoreAws().ServicecatalogTags()
input.RemoveTags = aws.StringSlice(keyvaluetags.New(o).IgnoreAws().Keys())
}
see portofolio resource tag update
| input := servicecatalog.DeleteProductInput{} | ||
| input.Id = aws.String(d.Id()) |
There was a problem hiding this comment.
lets put Id inline int the delete input struct
| }) | ||
| } | ||
|
|
||
| func TestAccAWSServiceCatalogProduct_tags(t *testing.T) { |
There was a problem hiding this comment.
try to follow the example used in TestAccAWSServiceCatalogPortfolio_Tags for parity
| return nil | ||
| } | ||
|
|
||
| func testAccAWSServiceCatalogProductConfig_basic(productSaltedName, bucketSaltedName, paSaltedName, tags string) string { |
There was a problem hiding this comment.
lets remove the tag from here and create a separate config for tags
| * `name` - (Required) The name of the product. | ||
| * `owner` - (Required) The owner of the product. | ||
| * `product_type` - (Required) The type of product. Valid values: `CLOUD_FORMATION_TEMPLATE` or `MARKETPLACE` | ||
| * `provisioning_artifact` - (Required) The configuration of the provisioning artifact. This object supports the following: |
There was a problem hiding this comment.
lets break down config blocks to a separate doc block, see cognito_user_pool for example. it will make reading easier.
|
@ahgittin Thank you for your work on this PR! I will be working on this before long. In order to expedite the process, I will likely make some changes. Make sure that you have checked the box "Allow edits from maintainers." (Don't worry though, you will receive all credit for your contribution and code!) Also, please coordinate with us before making any commits to this branch. Again, thank you for your help and we look forward to this popular addition to the AWS provider! |
|
@ahgittin @cloudsoft Thank you very much for all your work on this PR! I'm excited to work with you on this. However, in order to work with this PR, I need two things:
Since we look forward to adding this feature to the AWS Provider, we need you to make these changes by March 11, 2021. If you are unable to do this, we understand and thank you for your work. However, we will not be able to use this contribution. |
YakDriver
added
the
waiting-response
|
@YakDriver great to hear this. However there are a few stumbling blocks: (1) GitHub does not allow "Allow edits from maintainers" on this PR. I think it is due to isaacs/github#1681 . It might be best to open a new PR with all the contributed Service Catalog resources, squashed into a single commit to resolve (3). I can do this from my personal github instead of our organization which will resolve (1). Re (2) I will note the various contributors and provenance in the squashed commit message. |
ghost
removed
waiting-response
|
@ahgittin I'm very glad to hear from you! Might I suggest cherry picking? If you cherry pick to your own branch, you could squash many of the commits without losing contributions possibly. While I've done a lot of squashing, I've not squashed others commits so I'm not 100% sure it will maintain association of original contributor. But, if the history could be reduced to significantly fewer commits, that would be tremendous. It does not need to be 1. Squash Plan A: If the history has a consecutive group of 10 commits by the same authors, if you squash them, does it maintain the authorship? This would be ideal because the contributor line count would be maintained and the number of commits would be reduced. Squash Plan B: If squashing a group of commits does not maintain authorship, another possibility would be to maintain 1 (or a few) commits for each of the authors. The line count of each contributor would not be accurate but they would still show up as contributors. As for separate PRs versus a single one, it is often helpful to have them separated. The reviewer was absolutely correct that that is the normal practice. However, in this case, it has become difficult to tell which PR is dependent on which and which version of a given resource in which PR is the "latest and greatest." As a result, let's go against the normal approach and try a single PR/branch. To sum up, can we agree on this as a path forward?
Let me know if you have concerns. Also, as we are anxious to move forward, what does your timeframe look like? |
YakDriver
added
the
waiting-response
|
Good plan. Will do this ASAP.
In this case it might be easier to review as one PR because you can see the
similar code structure and how to test some you are necessarily using /
testing others. I hope that's what you find! Thanks for reviewing.
… |
ghost
removed
waiting-response
|
@YakDriver ^ Completed in #18074 . This can be closed in favour of that. |
|
This has been released in version 3.38.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! |
|
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. |
This PR adds support for the
aws_servicecatalog_product.This is included in #13797 which adds multiple Service Catalog resources, but this PR has the changes for that one resource isolated for easier review, done in such a way this PR can be merged on its own. As noted in #13797 this builds on #4980 and includes it.
Review comments from those PRs have been incorporated here, and tests updated based on the advice on #13797. (I hope I've done it right and included all that are needed!)
Community Note
Relates #13797
Closes #4980
Release note for CHANGELOG:
Output from acceptance testing: