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

Add configurable retry when reading PrivateLink information #246

Merged
merged 5 commits into from
Dec 18, 2023
+24 −10
Merged

Add configurable retry when reading PrivateLink information #246

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
06933b7
Add sleep/timeout to read func.
tbroden84 Dec 15, 2023
640dbbc
Lower sleep and timeout default values
tbroden84 Dec 15, 2023
2cfba6e
Update documentation
tbroden84 Dec 15, 2023
12455ef
Merge branch 'main' into privatelink-retries
tbroden84 Dec 18, 2023
d875ee8
Remove changelog from resource docs
tbroden84 Dec 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions cloudamqp/resource_cloudamqp_privatelink_aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ func resourcePrivateLinkAws() *schema.Resource {
"sleep": {
Type: schema.TypeInt,
Optional: true,
Default: 60,
Default: 10,
Description: "Configurable sleep in seconds between retries when enable PrivateLink",
},
"timeout": {
Type: schema.TypeInt,
Optional: true,
Default: 3600,
Default: 1800,
Description: "Configurable timeout in seconds when enable PrivateLink",
},
},
Expand Down Expand Up @@ -102,9 +102,11 @@ func resourcePrivateLinkAwsRead(d *schema.ResourceData, meta interface{}) error
var (
api = meta.(*api.API)
instanceID, _ = strconv.Atoi(d.Id()) // Uses d.Id() to allow import
sleep = d.Get("sleep").(int)
timeout = d.Get("timeout").(int)
)

data, err := api.ReadPrivatelink(instanceID)
data, err := api.ReadPrivatelink(instanceID, sleep, timeout)
if err != nil {
return err
}
Expand Down
8 changes: 5 additions & 3 deletions cloudamqp/resource_cloudamqp_privatelink_azure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,13 @@ func resourcePrivateLinkAzure() *schema.Resource {
"sleep": {
Type: schema.TypeInt,
Optional: true,
Default: 60,
Default: 10,
Description: "Configurable sleep in seconds between retries when enable PrivateLink",
},
"timeout": {
Type: schema.TypeInt,
Optional: true,
Default: 3600,
Default: 1800,
Description: "Configurable timeout in seconds when enable PrivateLink",
},
},
Expand Down Expand Up @@ -99,9 +99,11 @@ func resourcePrivateLinkAzureRead(d *schema.ResourceData, meta interface{}) erro
var (
api = meta.(*api.API)
instanceID, _ = strconv.Atoi(d.Id()) // Uses d.Id() to allow import
sleep = d.Get("sleep").(int)
timeout = d.Get("timeout").(int)
)

data, err := api.ReadPrivatelink(instanceID)
data, err := api.ReadPrivatelink(instanceID, sleep, timeout)
if err != nil {
return err
}
Expand Down
16 changes: 14 additions & 2 deletions docs/resources/privatelink_aws.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ PrivateLink, a new VPC will be created with subnet `10.52.72.0/24`.
<summary>
<i>Default PrivateLink firewall rule</i>
</summary>

```hcl
rules {
Description = "PrivateLink setup"
Expand All @@ -23,6 +24,7 @@ rules {
services = ["AMQP", "AMQPS", "HTTPS", "STREAM", "STREAM_SSL", "STOMP", "STOMPS", "MQTT", "MQTTS"]
}
```

</details>

Pricing is available at [cloudamqp.com](https://www.cloudamqp.com/plans.html)
Expand Down Expand Up @@ -58,6 +60,7 @@ resource "cloudamqp_privatelink_aws" "privatelink" {
]
}
```

</details>

<details>
Expand Down Expand Up @@ -91,16 +94,17 @@ resource "cloudamqp_privatelink_aws" "privatelink" {
]
}
```

</details>

## Argument Reference

* `instance_id` - (Required) The CloudAMQP instance identifier.
* `allowed_principals` - (Required) Allowed principals to access the endpoint service.
* `sleep` - (Optional) Configurable sleep time (seconds) when enable PrivateLink.
Default set to 60 seconds.
Default set to 10 seconds.
* `timeout` - (Optional) Configurable timeout time (seconds) when enable PrivateLink.
Default set to 3600 seconds.
Default set to 1800 seconds.

Allowed principals format: <br>
`arn:aws:iam::aws-account-id:root` <br>
Expand Down Expand Up @@ -190,4 +194,12 @@ resource "cloudamqp_security_firewall" "firewall_settings" {
]
}
```

</details>

## Changelog

List of changes made to this resource for different versions.

[v1.29.0](https://github.com/cloudamqp/terraform-provider-cloudamqp/releases/tag/v1.29.0) added
configurable retries when reading PrivateLink information.
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice touch but I think it can be hard to maintain properly. I think it is better to mention it in and concentrate our effort on keeping https://github.com/cloudamqp/terraform-provider-cloudamqp/blob/main/CHANGELOG.md as good as possible. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, tried it out here and liked it: a661e09#diff-6575c05c10a8f7d9c1d7cec73bd49538df9c9ace86edd8c659a7365aa79bd799
Just to get rid of all version input in the page as much as possible and keep it listed last. But could also become a hassle to keep track on.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah I saw https://github.com/cloudamqp/terraform-provider-cloudamqp/pull/247/files#diff-9efdf0ce35a9a79cd4ff4aad80f648515a179242612ffa732cba9066aeb61c37R283 now

I think the information serves a purpose, like we add new capabilities to a resource, someone reading it may not use the latest version, so it is good for them to see what version they need to be using to take advantage of a certain thing.

I think we should specify that directly in the documentation next to the thing it applies to. "Available since v1.2.9". Would it be possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here are previous examples:
As note boxes (looks better at registry and their markdown):
https://github.com/cloudamqp/terraform-provider-cloudamqp/blob/v1.28.0/docs/resources/plugin.md#cloudamqp_plugin

For each argument:
https://github.com/cloudamqp/terraform-provider-cloudamqp/blob/gcp-peering-retries/docs/resources/vpc_gcp_peering.md#argument-reference

Then perhaps due the latter part, but with Available since v1.2.9 for each argument/attribute and skip the note part.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, will be lots of boxes otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrong PR but just a concept. Example base on the discussion.
8def353

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, will be lots of boxes otherwise.

heh yeah, hence why I tried to put them in the changelog section instead.

Copy link
Member

Choose a reason for hiding this comment

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

Wrong PR but just a concept. Example base on the discussion. 8def353

Looks good but still think we should concentrate on only one changelog

16 changes: 14 additions & 2 deletions docs/resources/privatelink_azure.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,12 @@ Enable PrivateLink for a CloudAMQP instance hosted in Azure. If no existing VPC
enable PrivateLink, a new VPC will be created with subnet `10.52.72.0/24`.

-> **Note:** Enabling PrivateLink will automatically add firewall rules for the peered subnet.

<details>
<summary>
<i>Default PrivateLink firewall rule</i>
</summary>

```hcl
rules {
Description = "PrivateLink setup"
Expand All @@ -23,6 +25,7 @@ rules {
services = ["AMQP", "AMQPS", "HTTPS", "STREAM", "STREAM_SSL", "STOMP", "STOMPS", "MQTT", "MQTTS"]
}
```

</details>

Pricing is available at [cloudamqp.com](https://www.cloudamqp.com/plans.html) where you can also
Expand Down Expand Up @@ -91,6 +94,7 @@ resource "cloudamqp_privatelink_azure" "privatelink" {
]
}
```

</details>

## Argument Reference
Expand All @@ -99,9 +103,9 @@ resource "cloudamqp_privatelink_azure" "privatelink" {
* `approved_subscriptions` - (Required) Approved subscriptions to access the endpoint service.
See format below.
* `sleep` - (Optional) Configurable sleep time (seconds) when enable PrivateLink.
Default set to 60 seconds.
Default set to 10 seconds.
* `timeout` - (Optional) Configurable timeout time (seconds) when enable PrivateLink.
Default set to 3600 seconds.
Default set to 1800 seconds.

Approved subscriptions format (GUID): <br>
`XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX`
Expand Down Expand Up @@ -189,4 +193,12 @@ resource "cloudamqp_security_firewall" "firewall_settings" {
]
}
```

</details>

## Changelog

List of changes made to this resource for different versions.

[v1.29.0](https://github.com/cloudamqp/terraform-provider-cloudamqp/releases/tag/v1.29.0) added
configurable retries when reading PrivateLink information.
Copy link
Member

Choose a reason for hiding this comment

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

Same as https://github.com/cloudamqp/terraform-provider-cloudamqp/pull/246/files#r1427991445