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

feat: add support for event archive in EventBridge resource #17270

Merged
merged 26 commits into from
Feb 18, 2021

Conversation

heitorlessa
Copy link
Collaborator

@heitorlessa heitorlessa commented Jan 24, 2021

Intro

This PR adds support to create Archives in EventBridge.

Release note for CHANGELOG:

resource_aws_cloudwatch_event_archive - add new resource

Tasks

  • Initial CRUD implementation
  • Acceptance test
  • Update docs
  • Investigate answers to open questions

Open questions as a first time contributor

Q: How do I add support for import?

Update: Fix available in 5ab7d94

Tried following other resources that do support but it seems there's something wrongly parametrized:

// ...archive.go
	return &schema.Resource{
                ...
		Importer: &schema.ResourceImporter{
			State: schema.ImportStatePassthrough,
		},
// ...archive_test.go
....
{
    // also tried without Config key
    Config:            testAccAWSCloudWatchArchiveConfig(archiveName),
    ResourceName:      resourceName,
    ImportState:       true,
    ImportStateVerify: true,
},

Error

    resource_aws_cloudwatch_event_archive_test.go:78: Step 2/2 error running import: 
        Error: Error reading CloudWatch Events archive: InvalidParameter: 1 validation error(s) found.
        - minimum field size of 1, DescribeArchiveInput.ArchiveName.

Q: How do I fix acceptance test lint error in CI?

Can't see what's wrong by just looking at GH Actions logs. Maybe I missed that in the contrib checklist docs.

A: Running terrafmt diff --check --fmtcompat aws/resource_aws_cloudwatch_event_archive_test.go locally showed the error.

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 OR Closes #16067

Output from acceptance testing: WIP

$ make fmt && make testacc TESTARGS='-run=TestAccAWSCloudWatchArchive'       
==> Fixing source code with gofmt...
gofmt -s -w ./aws ./awsproviderlint/helper ./awsproviderlint/main.go ./awsproviderlint/passes
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSCloudWatchArchive -timeout 120m
=== RUN   TestAccAWSCloudWatchArchive_basic
=== PAUSE TestAccAWSCloudWatchArchive_basic
=== RUN   TestAccAWSCloudWatchArchive_update
=== PAUSE TestAccAWSCloudWatchArchive_update
=== CONT  TestAccAWSCloudWatchArchive_basic
=== CONT  TestAccAWSCloudWatchArchive_update
--- PASS: TestAccAWSCloudWatchArchive_basic (33.35s)
--- PASS: TestAccAWSCloudWatchArchive_update (55.17s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       57.862s

@heitorlessa heitorlessa requested a review from a team as a code owner January 24, 2021 17:10
@ghost ghost added size/L Managed by automation to categorize the size of a PR. service/cloudwatchevents labels Jan 24, 2021
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Jan 24, 2021
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome @heitorlessa 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTING guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

@heitorlessa heitorlessa marked this pull request as draft January 24, 2021 17:20
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/L Managed by automation to categorize the size of a PR. labels Jan 25, 2021
@ghost ghost added the documentation Introduces or discusses updates to documentation. label Jan 25, 2021
@heitorlessa heitorlessa marked this pull request as ready for review January 25, 2021 18:08
@heitorlessa heitorlessa changed the title [WIP] feat: add support for event archive in EventBridge resource feat: add support for event archive in EventBridge resource Jan 25, 2021
@heitorlessa
Copy link
Collaborator Author

All linting issues fixed - Ready for review

Copy link
Contributor

@bill-rich bill-rich left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few minor suggestions and questions. Also please add a .changelog file.

aws/resource_aws_cloudwatch_event_archive.go Outdated Show resolved Hide resolved
aws/resource_aws_cloudwatch_event_archive.go Show resolved Hide resolved
website/docs/r/cloudwatch_event_archive.html.markdown Outdated Show resolved Hide resolved
@bill-rich bill-rich self-assigned this Feb 18, 2021
@ghost ghost added the service/servicediscovery Issues and PRs that pertain to the servicediscovery service. label Feb 18, 2021
@heitorlessa
Copy link
Collaborator Author

heitorlessa commented Feb 18, 2021

Addressed all comments: Changelog entry, Test remaining attributes can be updated, ForceNew attribute, and Doc chars -> characters.

CI is failing due to Security Hub test file (non-existent) formatting unrelated to this PR, though Fixed it

…r-aws into main

* 'main' of https://github.com/hashicorp/terraform-provider-aws: (599 commits)
  Adds CHANGELOG
  Updates acceptance tests and documentation
  Update CHANGELOG.md for hashicorp#17650
  Removes redundant error check
  Update CHANGELOG.md for hashicorp#17645
  resource/aws_lb_listener_certificate: Prevent resource ID parsing error with IAM Server Certificate names containing underscores (hashicorp#17645)
  resource/aws_instance: Apply attribute waiter logic to iam_instance_profile attribute (hashicorp#17414)
  Update CHANGELOG.md for hashicorp#17534
  Adds multi-error handling to ElastiCache Cluster sweeper
  Excludes Clusters in Replication Groups from sweeper
  Adds check for deleting only member of replication group and does not retry
  Update CHANGELOG.md for hashicorp#17204
  Update docs for cloudfront_distribution resource
  fix(aws_ses_reciept_rule): include period in name validation regex
  Add CHANGELOG entry.
  r/aws_dx_gateway_association: Fix 'TestAccAwsDxGatewayAssociation_multiVpnGatewaysSingleAccount'.
  r/aws_ec2_transit_gateway: Increase deletion IncorrectState retry to 5 minutes.
  Update CHANGELOG.md after release
  fix if evaluation
  r/aws_dx_gateway_association: Modernize acceptance tests.
  ...
* main: (599 commits)
  Adds CHANGELOG
  Updates acceptance tests and documentation
  Update CHANGELOG.md for hashicorp#17650
  Removes redundant error check
  Update CHANGELOG.md for hashicorp#17645
  resource/aws_lb_listener_certificate: Prevent resource ID parsing error with IAM Server Certificate names containing underscores (hashicorp#17645)
  resource/aws_instance: Apply attribute waiter logic to iam_instance_profile attribute (hashicorp#17414)
  Update CHANGELOG.md for hashicorp#17534
  Adds multi-error handling to ElastiCache Cluster sweeper
  Excludes Clusters in Replication Groups from sweeper
  Adds check for deleting only member of replication group and does not retry
  Update CHANGELOG.md for hashicorp#17204
  Update docs for cloudfront_distribution resource
  fix(aws_ses_reciept_rule): include period in name validation regex
  Add CHANGELOG entry.
  r/aws_dx_gateway_association: Fix 'TestAccAwsDxGatewayAssociation_multiVpnGatewaysSingleAccount'.
  r/aws_ec2_transit_gateway: Increase deletion IncorrectState retry to 5 minutes.
  Update CHANGELOG.md after release
  fix if evaluation
  r/aws_dx_gateway_association: Modernize acceptance tests.
  ...
@ghost ghost added the service/securityhub Issues and PRs that pertain to the securityhub service. label Feb 18, 2021
Copy link
Contributor

@bill-rich bill-rich left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for taking a look at the comments.

@bill-rich bill-rich merged commit 7ed8ce4 into hashicorp:main Feb 18, 2021
@github-actions github-actions bot added this to the v3.29.0 milestone Feb 18, 2021
@ghost
Copy link

ghost commented Feb 19, 2021

This has been released in version 3.29.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 Mar 21, 2021

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 Mar 21, 2021
@breathingdust breathingdust removed the needs-triage Waiting for first response or review from a maintainer. label Sep 17, 2021
@justinretzolk justinretzolk added the partner Contribution from a partner. label May 16, 2024
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. partner Contribution from a partner. provider Pertains to the provider itself, rather than any interaction with AWS. service/securityhub Issues and PRs that pertain to the securityhub service. service/servicediscovery Issues and PRs that pertain to the servicediscovery service. size/XL 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.

New resource: aws_cloudwatch_event_archive
5 participants