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.

Sign up for GitHub

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

Jump to bottom

resource/cloudflare_email_routing_catch_all: switch to a dedicated scheme #1947

Merged
merged 5 commits into from
Oct 4, 2022
Merged

resource/cloudflare_email_routing_catch_all: switch to a dedicated scheme #1947

merged 5 commits into from
Oct 4, 2022

Conversation

yan12125
Copy link
Contributor

@yan12125 yan12125 commented Oct 3, 2022

Compared to a normal email routing rule, the catch-all rule:

  • Does not have a priority
  • For matchers, it accepts only type = "all", and there are no "field" and "value" properties
  • For actions, it accepts type = "drop" besides "forward" and "worker"

Ref: https://api.cloudflare.com/#email-routing-routing-rules-update-routing-rule
Ref: https://api.cloudflare.com/#email-routing-routing-rules-update-catch-all-rule

Fixes #1937

resource/cloudflare_email_routing_catch_all: switch to a dedicated sc…
b3b1594 
…heme

Compared to a normal email routing rule, the catch-all rule:

* Does not have a priority
* For matchers, it accepts only type = "all", and there are no "field"
  and "value" properties
* For actions, it accepts type = "drop" besides "forward" and "worker"

Ref: https://api.cloudflare.com/#email-routing-routing-rules-update-routing-rule
Ref: https://api.cloudflare.com/#email-routing-routing-rules-update-catch-all-rule

Fixes #1937
@yan12125 yan12125 requested a review from jacobbednarz as a code owner October 3, 2022 05:18
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2022 •
edited
Loading

changelog detected ✅

@yan12125
Copy link
Contributor Author

yan12125 commented Oct 3, 2022

As a side note, an API response for the catch-all rule contains priority. For example,

{
 "result": {
  "tag": "xxx",
  "name": "Catch-all rule",
  "matchers": [
   {
    "type": "all"
   }
  ],
  "actions": [
   {
    "type": "drop"
   }
  ],
  "enabled": true,
  "priority": 2147483647
 },
 "success": true,
 "errors": [],
 "messages": []
}

I'm not sure if priority should be added as a computed field in the new scheme or not.

@jacobbednarz
Copy link
Member

I'm not sure if priority should be added as a computed field in the new scheme or not.

if it's not in the public API, we'll need to wait until it's declared stable (and documented) before we implement it here.

are you able to run make docs for this locally? the automatic generation of documentation will need to be kicked since we've updated the fields.

jacobbednarz
jacobbednarz reviewed Oct 3, 2022
View reviewed changes
internal/provider/schema_cloudflare_email_routing_catch_all.go Outdated
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"type": {
Description: "Type of matcher.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Description: "Type of matcher.",
Description: fmt.Sprintf("Type of matcher. %s", renderAvailableDocumentationValuesStringSlice([]string{"all"})),

(don't forget to update your imports for fmt)

jacobbednarz
jacobbednarz reviewed Oct 3, 2022
View reviewed changes
internal/provider/schema_cloudflare_email_routing_catch_all.go Outdated
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"type": {
Description: "Type of supported action.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Description: "Type of supported action.",
Description: fmt.Sprintf("Type of supported action. %s", renderAvailableDocumentationValuesStringSlice([]string{"drop", "forward", "worker"})),

jacobbednarz
jacobbednarz reviewed Oct 3, 2022
View reviewed changes
internal/provider/schema_cloudflare_email_routing_catch_all.go Outdated
ValidateFunc: validation.StringInSlice([]string{"drop", "forward", "worker"}, true),
},
"value": {
Description: "An array with items in the following form.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Description: "An array with items in the following form.",
Description: "A list with items in the following form.",

@yan12125
Copy link
Contributor Author

yan12125 commented Oct 4, 2022

Thanks! Descriptions are improved.

if it's not in the public API, we'll need to wait until it's declared stable (and documented) before we implement it here.

Got it! Then I will omit the priority in codes for now.

are you able to run make docs for this locally? the automatic generation of documentation will need to be kicked since we've updated the fields.

docs/resources/email_routing_catch_all.md is updated and committed. However, during make docs, docs/data-sources/devices.md is also modified, and changes in 924f1e8#diff-410ef65dd8c8160d34c379f6a0d303020f23b6631bdd91955b2d27962e7025e9 are reverted. I assume that's unrelaed to this PR, so I didn't commit it.

@jacobbednarz jacobbednarz merged commit 821c1e1 into cloudflare:master Oct 4, 2022
@jacobbednarz
Copy link
Member

thanks @yan12125 💯 you rock!

@github-actions github-actions bot added this to the v3.25.0 milestone Oct 4, 2022
github-actions bot pushed a commit that referenced this pull request Oct 4, 2022
Update CHANGELOG.md for #1947
4d5dd10
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2022

This functionality has been released in v3.25.0 of the Terraform Cloudflare 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. Thank you!

@yan12125 yan12125 deleted the issue1937 branch October 5, 2022 00:10
@yan12125
Copy link
Contributor Author

yan12125 commented Oct 5, 2022

Cool, thanks!

limejuny added a commit to limejuny/terraform-provider-cloudflare that referenced this pull request May 18, 2023
@limejuny
add drop options to email_routing_rule
42b745e 
Detail
======
 - the `action` should be one of `forward`, `worker`, `drop`, but there
   was no `drop` in latest provider.
 - ref: cloudflare#1947
This was referenced May 18, 2023
Closed
Merged
limejuny added a commit to limejuny/terraform-provider-cloudflare that referenced this pull request Jul 8, 2023
@limejuny
add drop options to email_routing_rule
9891284 
Detail
======
 - the `action` should be one of `forward`, `worker`, `drop`, but there
   was no `drop` in latest provider.
 - ref: cloudflare#1947
limejuny added a commit to limejuny/terraform-provider-cloudflare that referenced this pull request Nov 21, 2023
@limejuny
add drop options to email_routing_rule
4c91c1d 
Detail
======
 - the `action` should be one of `forward`, `worker`, `drop`, but there
   was no `drop` in latest provider.
 - ref: cloudflare#1947
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobbednarz jacobbednarz jacobbednarz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.25.0
Development

Successfully merging this pull request may close these issues.

Cannot specify type = drop for cloudflare_email_routing_catch_all
2 participants
@yan12125 @jacobbednarz