[WIP] Add aws_guardduty_filter provider #11086
Conversation
ghost
added
needs-triage
|
Hello, and thank you for your contribution! This project recently migrated to the standalone Terraform Plugin SDK. While the migration helps speed up future feature requests and bug fixes to the Terraform AWS Provider's interface with Terraform, it has the unfortunate consequence of requiring minor changes to pull requests created using the old SDK. This pull request appears to include the Go import path To resolve this situation without losing any existing work, you may be able to Git rebase your branch against the current master branch (example below); replacing any remaining old import paths with the newer ones. $ git fetch --all
$ git rebase origin/masterAnother option is to create a new branch from the current master with the same code changes (replacing the import paths), submit a new pull request, and close this existing pull request. We apologize for this inconvenience and appreciate your effort. Thank you for contributing and helping make the Terraform AWS Provider better for everyone. |
There was a problem hiding this comment.
Thanks @phundisk, this is a good start. I have a few comments.
We try to keep the Terraform parameters consistent with the AWS API parameters, and I see that there are a few places where you've changed it a bit.
I've found that writing the acceptance tests as you go alongside the resource code can be very helpful. YMMV, of course.
You'll also need to rebase on the current code as the bot above mentioned
gdavison
added
new-resource
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| "detector_id": { | ||
| Type: schema.TypeString, |
There was a problem hiding this comment.
Please add a validator with a min length of 1 and max length 300
| Type: schema.TypeString, | ||
| Required: true, | ||
| }, | ||
| "description": { |
There was a problem hiding this comment.
This isn't required by the API, so Terraform shouldn't require it. It also has a max length of 512.
| Required: true, | ||
| ForceNew: true, | ||
| }, | ||
| "name": { |
| Type: schema.TypeString, | ||
| Required: true, | ||
| }, | ||
| "auto_archive": { |
There was a problem hiding this comment.
This should be action, and allow either NOOP or ARCHIVE
| Type: schema.TypeInt, | ||
| Required: true, | ||
| }, | ||
| "filters": { |
There was a problem hiding this comment.
This should be finding_criteria to match the API
| Type: schema.TypeList, | ||
| Required: true, | ||
| Elem: &schema.Resource{ | ||
| Schema: map[string]*schema.Schema{ |
There was a problem hiding this comment.
These elements should match the non-deprecated values of the guardduty.Condition struct. You can make all of the attributes Optional, and then combinations of ConflictsWith, ExactlyOneOf, and AtLeastOneOf as applicable for the filter combinations.
|
Hey I am going to take a look at the comments this weekend and get the PR is a good state! Thanks so much for your comments @glasser |
|
The one at https://github.com/terraform-providers/terraform-provider-aws/pull/10676/files looks good and a bit more robust than my initial PR. It might make more sense to get that one merged. I can take a look this weekend if we decide to go with my initial PR instead |
|
As mentioned above, we're going to go with #10676. I'm going to close this ticket |
|
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
locked and limited conversation to collaborators
Attempted to add provider per #9647
Still need to add tests and tags resource but looking for general feedback on my approach and any go best practices since I am new to Go.
Community Note
Closes #9647
Release note for CHANGELOG:
Output from acceptance testing: