r/aws_licensemanager: Add aws_licensemanager_license_configuration resource

[gazoakley]

Reference: #6654

Changes proposed in this pull request:

• New Resource: aws_licensemanager_license_configuration

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSLicenseManagerLicenseConfiguration'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSLicenseManagerLicenseConfiguration -timeout 120m
=== RUN   TestAccAWSLicenseManagerLicenseConfiguration_basic
=== PAUSE TestAccAWSLicenseManagerLicenseConfiguration_basic
=== CONT  TestAccAWSLicenseManagerLicenseConfiguration_basic
--- PASS: TestAccAWSLicenseManagerLicenseConfiguration_basic (10.13s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	10.172s

[bflad]

Thanks so much as usual, @gazoakley 👍 A few little things then we can get this in.

[gazoakley]

Thanks @bflad - I'll address the review comments shortly. From a design perspective I'm thinking license_rules could be turned into a map since it can only contain strings in the format of #RuleType=RuleValue (e.g. #minimumSockets=3). That feels more user friendly to me, but I think it breaks away from the principal of matching the API. Any preference? At the very least I want to add validation for the strings and document the possible RuleTypes.

There also seems to be a bug in the License Manager API that prevents removing license_count - the API doesn't allow resetting the license count to unlimited. The same problem is visible in the AWS console too. I could:

• Try and get AWS to fix the API and console before adding the resource
• Set ForceNew on that attribute, but that seems overkill when it otherwise works
• Throw an error when applying a change that removes license_count asking the user to taint the resource
• Could we have a ForceNewFunc to allow conditionally re-creating resources at plan time?

[bflad]

From a design perspective I'm thinking license_rules could be turned into a map since it can only contain strings in the format of #RuleType=RuleValue (e.g. #minimumSockets=3). That feels more user friendly to me, but I think it breaks away from the principal of matching the API. Any preference?

I would only do this if we can get confirmation from the AWS License Manager service team that the format will always follow a single #KEY=VALUE, map structure for the string field. One would hope they would use a map in the API if it would always be a map like that. I could see the potential for other logic additions such as OR or other operators. I have also have concerns that trying to inject validation could leave us either with a maintenance burden trying to keep up or needing to remove it if arbitrary rules are introduced. I these are all questions of stability that the service team could also confirm.

There also seems to be a bug in the License Manager API that prevents removing license_count - the API doesn't allow resetting the license count to unlimited. The same problem is visible in the AWS console too.

Does the API throw a decent error? Is this planned on getting fixed? If it will be fixed, at most we should create a better error message, otherwise we should just pass the existing error through and potentially document the upstream issue. This doesn't seem like it should hold up releasing the resource or block other working update behavior with ForceNew.

[gazoakley]

I would only do this if we can get confirmation from the AWS License Manager service team that the format will always follow a single #KEY=VALUE, map structure for the string field. One would hope they would use a map in the API if it would always be a map like that. I could see the potential for other logic additions such as OR or other operators. I have also have concerns that trying to inject validation could leave us either with a maintenance burden trying to keep up or needing to remove it if arbitrary rules are introduced. I these are all questions of stability that the service team could also confirm.

The API throws an error message if you don't specify it in that format: Expected format is #RuleType=RuleValue. I get that if it can't be guaranteed that they won't change the format then using a map could cause problems in the future. I've added a ValidateFunc for now to check the format (which would be easy to strip out if they start to allow logic operators) - maybe that feels like the best compromise?

Does the API throw a decent error? Is this planned on getting fixed? If it will be fixed, at most we should create a better error message, otherwise we should just pass the existing error through and potentially document the upstream issue. This doesn't seem like it should hold up releasing the resource or block other working update behavior with ForceNew.

It just silently returns success if you don't specify a value for opts.LicenseCount and then doesn't make a change. The console specifies "LicenseCount": null (checking Chrome DevTools), but that doesn't reset the value either. I'm not sure aws-sdk-go allows me to pass that value as null because I can't see the request body for JSON requests in the debug logs (although I can see JSON responses). For now I could document that resetting this value requires tainting the resource?

[bflad]

I've added a ValidateFunc for now to check the format (which would be easy to strip out if they start to allow logic operators) - maybe that feels like the best compromise?

Fantastic compromise. 😄 Someone will complain if/when its out of date.

For now I could document that resetting this value requires tainting the resource?

I think this is good for now. 👍

[gazoakley]

@bflad: Updated 😄

[bflad]

LGTM, thanks @gazoakley! 🚀

--- PASS: TestAccAWSLicenseManagerLicenseConfiguration_basic (4.52s)
--- PASS: TestAccAWSLicenseManagerLicenseConfiguration_update (7.36s)

[bflad]

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

[ghost]

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!