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

Serverless Application Repository initial support #15874

Merged
merged 46 commits into from
Nov 25, 2020
Merged

Conversation

gdavison
Copy link
Contributor

Adds AWS Serverless Application Repository applications to the Terraform provider. Replaces #5961, since I'm no longer working with that fork of the provider repo.

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

Closes #3981

Release note for CHANGELOG:

* **New Data Source:** `aws_serverlessrepository_application`
* **New Resource:** `aws_serverlessrepository_stack`

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAwsServerlessRepositoryStack_\|TestAccDataSourceAwsServerlessRepositoryApplication_'

--- PASS: TestAccDataSourceAwsServerlessRepositoryApplication_Versioned (100.81s)
--- PASS: TestAccDataSourceAwsServerlessRepositoryApplication_Basic (102.51s)
--- PASS: TestAccAwsServerlessRepositoryStack_versioned (163.66s)
--- PASS: TestAccAwsServerlessRepositoryStack_tagged (186.77s)
--- PASS: TestAccAwsServerlessRepositoryStack_basic (186.93s)
--- PASS: TestAccAwsServerlessRepositoryStack_versionUpdate (264.62s)
--- PASS: TestAccAwsServerlessRepositoryStack_update (297.12s)

Graham Davison and others added 26 commits September 3, 2019 23:50
…o separate function; will be reused for Update"

This was a premature simplification
This confirms that no tags show up in the state when no tags are added to the resource
@ghost ghost added size/XXL Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. provider Pertains to the provider itself, rather than any interaction with AWS. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Oct 27, 2020
@ghost ghost added the service/cloudformation Issues and PRs that pertain to the cloudformation service. label Nov 20, 2020
@gdavison gdavison marked this pull request as ready for review November 20, 2020 21:53
@gdavison gdavison requested a review from a team as a code owner November 20, 2020 21:53
@gdavison gdavison added new-data-source Introduces a new data source. new-resource Introduces a new resource. labels Nov 20, 2020
@bflad bflad self-assigned this Nov 23, 2020
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

First round of changes, I think overall this is handling this unique service pretty well though 👍

Comment on lines 27 to 29
Importer: &schema.ResourceImporter{
State: schema.ImportStatePassthrough,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Resource import support is missing acceptance testing, e.g.

{
	ResourceName:      resourceName,
	ImportState:       true,
	ImportStateVerify: true,
},

@@ -0,0 +1,50 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation is missing section on resource import.

Comment on lines 141 to 152
getApplicationInput := &serverlessrepository.GetApplicationInput{
ApplicationId: aws.String(d.Get("application_id").(string)),
}

_, ok := d.GetOk("semantic_version")
if !ok {
getApplicationOutput, err := serverlessConn.GetApplication(getApplicationInput)
if err != nil {
return err
}
d.Set("semantic_version", getApplicationOutput.Version.SemanticVersion)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This will likely cause issues on import since it is currently set to ImportStatePassthrough -- will need to add these to custom import function where they are present in the ID then passed into the correct attributes for the Read function.

return nil
}
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

To help operators and code maintainers with context around error messages:

Suggested change
return err
return fmt.Errorf("error describing CloudFormation Stack (%s): %w", d.Id(), err)

See also: #15892

Deploys an application from the Serverless Application Repository.
---

# Resource: aws_serverlessrepository_application
Copy link
Contributor

@bflad bflad Nov 23, 2020

Choose a reason for hiding this comment

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

Suggested change
# Resource: aws_serverlessrepository_application
# Resource: aws_serverlessrepository_stack

(Or whatever is decided with potentially lengthening the naming)

See also: #15842


# Resource: aws_serverlessrepository_application

Deploys an application from the Serverless Application Repository.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Deploys an application from the Serverless Application Repository.
Deploys a CloudFormation Stack using an Application from the Serverless Application Repository.

## Example Usage

```hcl
resource "aws_serverlessrepository_application" "postgres-rotator" {
Copy link
Contributor

@bflad bflad Nov 23, 2020

Choose a reason for hiding this comment

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

(Or whatever is decided with potentially lengthening the naming)

Suggested change
resource "aws_serverlessrepository_application" "postgres-rotator" {
resource "aws_serverlessrepository_stack" "postgres-rotator" {

aws/provider.go Outdated
@@ -892,6 +893,7 @@ func Provider() *schema.Provider {
"aws_securityhub_member": resourceAwsSecurityHubMember(),
"aws_securityhub_product_subscription": resourceAwsSecurityHubProductSubscription(),
"aws_securityhub_standards_subscription": resourceAwsSecurityHubStandardsSubscription(),
"aws_serverlessrepository_stack": resourceAwsServerlessRepositoryStack(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I apologize if we've discussed this before, but these SAR resources and data sources should probably be expanded to the full service naming per the Contribution Guide, unless there is a blocking reason not to (length be darned 🙁 ).

"aws_serverlessapplicationrepository_stack": resourceAwsServerlessApplicationRepositoryStack(),

We may also want to consider adding "CloudFormation" Stack in here to match the API CreateCloudFormationStack naming, in case SAR releases some other form of "stacks" (such as its own).

"aws_serverlessapplicationrepository_cloudformation_stack": resourceAwsServerlessApplicationRepositoryCloudFormationStack(),

Comment on lines 169 to 170
var e *resource.NotFoundError
if errors.As(err, &e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since its not being used yet (only tfresource.TimedOut), it might be good time to update

func NotFound(err error) bool {
_, ok := err.(*resource.NotFoundError)
return ok
}
with errors.As and use it here. 👍

@bflad bflad self-requested a review November 25, 2020 15:31
@bflad bflad added this to the v3.18.0 milestone Nov 25, 2020
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Excellent! 🚀

Output from acceptance testing in AWS Commercial:

--- PASS: TestAccDataSourceAwsServerlessApplicationRepositoryApplication_Basic (52.09s)
--- PASS: TestAccDataSourceAwsServerlessApplicationRepositoryApplication_Versioned (92.29s)

--- PASS: TestAccAwsServerlessApplicationRepositoryCloudFormationStack_disappears (138.27s)
--- PASS: TestAccAwsServerlessApplicationRepositoryCloudFormationStack_paired (142.73s)
--- PASS: TestAccAwsServerlessApplicationRepositoryCloudFormationStack_basic (164.41s)
--- PASS: TestAccAwsServerlessApplicationRepositoryCloudFormationStack_update (232.10s)
--- PASS: TestAccAwsServerlessApplicationRepositoryCloudFormationStack_versioned (262.14s)
--- PASS: TestAccAwsServerlessApplicationRepositoryCloudFormationStack_Tags (271.30s)

Output from acceptance testing in AWS GovCloud (US):

--- PASS: TestAccDataSourceAwsServerlessApplicationRepositoryApplication_Basic (86.85s)
--- PASS: TestAccDataSourceAwsServerlessApplicationRepositoryApplication_Versioned (97.04s)

--- PASS: TestAccAwsServerlessApplicationRepositoryCloudFormationStack_disappears (129.86s)
--- PASS: TestAccAwsServerlessApplicationRepositoryCloudFormationStack_basic (143.42s)
--- PASS: TestAccAwsServerlessApplicationRepositoryCloudFormationStack_paired (144.45s)
--- PASS: TestAccAwsServerlessApplicationRepositoryCloudFormationStack_update (226.49s)
--- PASS: TestAccAwsServerlessApplicationRepositoryCloudFormationStack_Tags (250.56s)
--- PASS: TestAccAwsServerlessApplicationRepositoryCloudFormationStack_versioned (257.11s)

_, ok := err.(*resource.NotFoundError)
return ok
var e *resource.NotFoundError
return errors.As(err, &e)
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

@gdavison gdavison merged commit 18279c6 into master Nov 25, 2020
@gdavison gdavison deleted the serverless_app_repo branch November 25, 2020 20:57
gdavison added a commit that referenced this pull request Nov 25, 2020
@ghost
Copy link

ghost commented Nov 25, 2020

This has been released in version 3.18.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!

@ghost
Copy link

ghost commented Dec 26, 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 as resolved and limited conversation to collaborators Dec 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. new-data-source Introduces a new data source. new-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. service/cloudformation Issues and PRs that pertain to the cloudformation service. size/XXL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for AWS Serverless Application Model and Repository
2 participants