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

Add SES resource #5387

Merged
merged 7 commits into from
Jun 26, 2016
Merged

Add SES resource #5387

merged 7 commits into from
Jun 26, 2016

Conversation

yissachar
Copy link
Contributor

This PR adds support for the SES resource.

There are two outstanding issues that make this a WIP:

  1. According to the AWS docs, a newly created receipt rule will be enabled and have spam and virus scanning enabled by default. I've confirmed this is the case when creating receipt rules through the web UI. However, when creating a receipt rule through Terraform with this branch that is not the case. Instead enabled and scan_enabled have to be manually set in order to prevent them from defaulting to false. I don't know why this is happening.
  2. Currently there is no support for controlling the order of actions for a receipt rule. I don't see a good way to map the AWS model (a single ordered list of actions, where each action can be one of the specified types, e.g. AddHeaderAction or S3Action) to the Terraform model. I am open to suggestions as to how this can be accomplished.

@yissachar
Copy link
Contributor Author

Any thoughts of how to model the ReceiptRule list of actions to a Terraform structure? I can't think of a good way to do this within the current Terraform model.

@yissachar
Copy link
Contributor Author

Apparently the first issue was simply a documentation issue. The docs have been updated to reflect the correct default values, so this is no longer a problem.

@blalor
Copy link
Contributor

blalor commented Apr 10, 2016

Any updates on this? I'd really like to see SES support in Terraform.

@yissachar
Copy link
Contributor Author

I've been busy with a few other things and haven't had time to devote to this PR. My current plan is to add a position value to each action that will override the default ordering. I haven't fully thought through the ramifications, but I believe this change would work to fix the current ordering issue. When I have more time available, I will begin implementing this and see if it works properly or not.

resource "aws_ses_receipt_rule" "actions" {
    name = "actions"
    rule_set_name = "${aws_ses_receipt_rule_set.test.rule_set_name}"

    add_header_action {
      header_name = "Added-By"
      header_value = "Terraform"
      position = 2
    }

   add_header_action {
      header_name = "Added-By"
      header_value = "Terraform"
      position = 1
    }
}

@yissachar yissachar mentioned this pull request May 26, 2016
@yissachar yissachar changed the title [WIP] Add SES resource Add SES resource Jun 1, 2016
@yissachar
Copy link
Contributor Author

All right, I've managed to get the position changes in, and now everything works.

@radeksimko I've rebased against master and tested everything. This should be ready to go now.

@yissachar
Copy link
Contributor Author

@radeksimko I don't want this PR to end up neglected. Are you planning to take a look at this? Is there anything else I can do to help it get merged?

@stack72
Copy link
Contributor

stack72 commented Jun 23, 2016

Hi @yissachar

Apologies for this slipping through the cracks - please can this be rebased against master?

We can try and get it reviewed asap

Paul

}
}

func resourceAwsSesActiveReceiptRuleSetCreate(d *schema.ResourceData, meta interface{}) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

IF all this func does is call Update, then line 13 can change to:

Create: resourceAwsSesActiveReceiptRuleSetUpdate,

@stack72
Copy link
Contributor

stack72 commented Jun 23, 2016

Hi @yissachar

Just had an initial review of this - the functionality looks good to me. When you have rebased, I can get the tests running and then see if we are in a state for a merge

Thanks for the work here so far

Paul

@stack72 stack72 self-assigned this Jun 23, 2016
@stack72 stack72 added the waiting-response An issue/pull request is waiting for a response from the community label Jun 23, 2016
@yissachar
Copy link
Contributor Author

Hi @stack72, thanks for taking a look at this!

I've rebased against master and pushed some commits to address the points you've brought up. I've also left some comments about things that I don't believe are issues.

@stack72
Copy link
Contributor

stack72 commented Jun 26, 2016

Hi @yissachar

Thanks for making the changes here :) This LGTM! The test results are as follows:

terraform [yissachar-ses-resource] % make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSSESActiveReceiptRuleSet_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSSESActiveReceiptRuleSet_ -timeout 120m
=== RUN   TestAccAWSSESActiveReceiptRuleSet_basic
--- PASS: TestAccAWSSESActiveReceiptRuleSet_basic (15.93s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    15.968s



terraform [yissachar-ses-resource] % make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSSESReceiptFilter_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSSESReceiptFilter_ -timeout 120m
=== RUN   TestAccAWSSESReceiptFilter_basic
--- PASS: TestAccAWSSESReceiptFilter_basic (17.12s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    17.153s



terraform [yissachar-ses-resource] % make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSSESReceiptRuleSet_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSSESReceiptRuleSet_ -timeout 120m
=== RUN   TestAccAWSSESReceiptRuleSet_basic
--- PASS: TestAccAWSSESReceiptRuleSet_basic (17.29s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    17.314s


terraform [yissachar-ses-resource] % make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSSESReceiptRule_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSSESReceiptRule_ -timeout 120m
=== RUN   TestAccAWSSESReceiptRule_basic
--- PASS: TestAccAWSSESReceiptRule_basic (22.23s)
=== RUN   TestAccAWSSESReceiptRule_order
--- PASS: TestAccAWSSESReceiptRule_order (33.51s)
=== RUN   TestAccAWSSESReceiptRule_actions
--- PASS: TestAccAWSSESReceiptRule_actions (43.78s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    99.533s

@stack72 stack72 merged commit 1bd8b44 into hashicorp:master Jun 26, 2016
@yissachar yissachar deleted the ses-resource branch June 26, 2016 21:50
mcanevet pushed a commit to mcanevet/terraform that referenced this pull request Jun 27, 2016
* Add SES resource

* Detect ReceiptRule deletion outside of Terraform

* Handle order of rule actions

* Add position field to docs

* Fix hashes, add log messages, and other small cleanup

* Fix rebase issue

* Fix formatting
@bobbydeveaux
Copy link
Contributor

excellent- thanks to all for this!


* `header_name` - (Required) The name of the header to add
* `header_value` - (Required) The value of the header to add
* `position` - (Required) The position of the action in the receipt rule
Copy link
Contributor

Choose a reason for hiding this comment

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

@yissachar This says that position is required but the example doesn't include it. Which is right? Taking a quick glance at the code it does look like it's required. It might also be useful to point out that this should be an integer but I'm not sure if that's done elsewhere in the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Position is required.

@ghost
Copy link

ghost commented Apr 24, 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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement provider/aws waiting-response An issue/pull request is waiting for a response from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants