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

Added guardduty filter resource #10676

Closed

Conversation

graffzon
Copy link
Contributor

@graffzon graffzon commented Oct 30, 2019

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" comments, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #0000 (Sorry, I didn't get, shall I create an issue for this PR or the PR on its own is enough?)

Release note for CHANGELOG:

Added a GuardDuty Filter resource.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSGuardDuty/Filter'
    --- PASS: TestAccAWSGuardDuty/Filter (34.36s)
        --- PASS: TestAccAWSGuardDuty/Filter/basic (16.44s)
        --- PASS: TestAccAWSGuardDuty/Filter/import (17.92s)
...

@graffzon graffzon requested a review from a team October 30, 2019 13:48
@ghost ghost added provider Pertains to the provider itself, rather than any interaction with AWS. service/guardduty Issues and PRs that pertain to the guardduty service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. documentation Introduces or discusses updates to documentation. size/XL Managed by automation to categorize the size of a PR. labels Oct 30, 2019
@graffzon
Copy link
Contributor Author

Hi there! I see you have hundreds of pull requests, folks :( Is there any chance I can speed up a review for this one?

Copy link
Contributor

@gdavison gdavison left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @graffzon, it looks good. I have a few changes for you to make

website/docs/r/guardduty_filter.html.markdown Show resolved Hide resolved
aws/resource_aws_guardduty_filter.go Show resolved Hide resolved
aws/resource_aws_guardduty_filter.go Outdated Show resolved Hide resolved
aws/resource_aws_guardduty_filter.go Outdated Show resolved Hide resolved
aws/resource_aws_guardduty_filter.go Outdated Show resolved Hide resolved
Config: testAccGuardDutyFilterConfig_full(),
Check: resource.ComposeTestCheckFunc(
testAccCheckAwsGuardDutyFilterExists(resourceName),
resource.TestCheckResourceAttrSet(resourceName, "detector_id"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Use resource.TestCheckResourceAttrPair to verify that the correct value is set

@gdavison gdavison self-assigned this Feb 19, 2020
@gdavison gdavison added the new-resource Introduces a new resource. label Feb 19, 2020
@gdavison
Copy link
Contributor

gdavison commented Mar 9, 2020

Hi @graffzon, are you still interested in working on this PR?

suzuki-shunsuke added a commit to suzuki-shunsuke/terraform-provider-aws that referenced this pull request Jun 30, 2020
suzuki-shunsuke added a commit to suzuki-shunsuke/terraform-provider-aws that referenced this pull request Jun 30, 2020
suzuki-shunsuke added a commit to suzuki-shunsuke/terraform-provider-aws that referenced this pull request Jun 30, 2020
suzuki-shunsuke added a commit to suzuki-shunsuke/terraform-provider-aws that referenced this pull request Jun 30, 2020
suzuki-shunsuke added a commit to suzuki-shunsuke/terraform-provider-aws that referenced this pull request Jun 30, 2020
suzuki-shunsuke added a commit to suzuki-shunsuke/terraform-provider-aws that referenced this pull request Jun 30, 2020
suzuki-shunsuke added a commit to suzuki-shunsuke/terraform-provider-aws that referenced this pull request Jun 30, 2020
suzuki-shunsuke added a commit to suzuki-shunsuke/terraform-provider-aws that referenced this pull request Jun 30, 2020
suzuki-shunsuke added a commit to suzuki-shunsuke/terraform-provider-aws that referenced this pull request Jun 30, 2020
@suzuki-shunsuke
Copy link
Contributor

@graffzon @gdavison
Thank you for your great work.
I'm interested in this pull request, but this seems to be pending.

Can I contribute to this pull request?
If I can, how should I do?

I checked out this pull request and added some commits according to the review comments.

Should I create a new pull request? Or is there any other way?

Thank you.

@graffzon
Copy link
Contributor Author

graffzon commented Jul 6, 2020

Hi folks, my apologies for keeping silence. @suzuki-shunsuke if you have time and willingness to contribute to this PR - I'd be happy. Unfortunately I don't have a prepared setup to test the GuardDuty resources anymore. However, if it will hang for a couple more months, I might have time to come back to it 😅

@graffzon
Copy link
Contributor Author

graffzon commented Jul 6, 2020

@suzuki-shunsuke I found a way to merge your changes into this PR, thanks a lot for your work.
@gdavison could you please take another look at the PR after the changes made by @suzuki-shunsuke ?

@graffzon graffzon requested a review from gdavison July 6, 2020 16:18
@suzuki-shunsuke
Copy link
Contributor

suzuki-shunsuke commented Jul 7, 2020

Merging master into feature branch
@graffzon
Copy link
Contributor Author

graffzon commented Jul 7, 2020

@suzuki-shunsuke Thanks for being that quick :)
I didn't manage to merge your PR because it contained an update from upstream and produced a weird changes for me, so I just updated my fork from the upstream and added the code you implemented in your commits, tagging you in the commits' description. By some reason it still fails, I will have a look later today

@suzuki-shunsuke
Copy link
Contributor

suzuki-shunsuke commented Jul 7, 2020

@graffzon

I didn't manage to merge your PR because it contained an update from upstream and produced a weird changes for me

Would you cherry pick 045794d and 53d0e17 instead of bb3819c 255ff95?

$ git remote add suzuki-shunsuke https://github.com/suzuki-shunsuke/terraform-provider-aws 
$ git remote -v
origin	https://github.com/graffzon/terraform-provider-aws (fetch)
origin	https://github.com/graffzon/terraform-provider-aws (push)
suzuki-shunsuke	https://github.com/suzuki-shunsuke/terraform-provider-aws (fetch)
suzuki-shunsuke	https://github.com/suzuki-shunsuke/terraform-provider-aws (push)

$ git fetch suzuki-shunsuke
$ git reset HEAD~2 --hard # remove bb3819c 255ff95
$ git cherry-pick 045794d 53d0e17
$ git push origin added-guardduty-filter-resource --force

@graffzon graffzon force-pushed the added-guardduty-filter-resource branch from 54c10e1 to e313e2b Compare August 11, 2020 19:03
@graffzon graffzon force-pushed the added-guardduty-filter-resource branch 2 times, most recently from 8dcad82 to e313e2b Compare August 11, 2020 19:09
@graffzon
Copy link
Contributor Author

Thanks for your tremendous effort @gdavison and @suzuki-shunsuke !!! 🙏
I merged the branch from @suzuki-shunsuke and rebased from the latest master.
Just not sure why it says only 4 checks when I'm pretty sure I saw smth like 12 before rebasing. I'm not very familiar with the Github Checks, it's a rather new feature that I haven't used yet. Might it have just skipped some checks? Like I don't actually see the tests being ran e.g.
And about the review, sorry I've dropped out from the review process, is everything incorporated now?

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Aug 11, 2020
@gdavison
Copy link
Contributor

Thanks again for all the work you've put in, @graffzon and @suzuki-shunsuke. There are a few remaining changes we'd like to see before merging the PR.

If one of you would like to make the changes go ahead. Otherwise, I can make the changes next week (24 August) so that we can get it in for the release that week.

@gdavison gdavison added the waiting-response Maintainers are waiting on response from community or contributor. label Aug 18, 2020
@gdavison gdavison added this to the v3.5.0 milestone Sep 2, 2020
@ghost
Copy link

ghost commented Sep 3, 2020

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

@graffzon graffzon deleted the added-guardduty-filter-resource branch September 7, 2020 10:28
@ghost
Copy link

ghost commented Oct 3, 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!

@hashicorp hashicorp locked as resolved and limited conversation to collaborators Oct 3, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
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-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. service/guardduty Issues and PRs that pertain to the guardduty 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.

None yet

4 participants