Skip to content

feat: add built-in rate limiting policy configuration support#237

Merged
rosslovas merged 2 commits into
mainfrom
ross/add-built-in-rate-limiting-policies
Jul 24, 2026
Merged

feat: add built-in rate limiting policy configuration support#237
rosslovas merged 2 commits into
mainfrom
ross/add-built-in-rate-limiting-policies

Conversation

@rosslovas

@rosslovas rosslovas commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Adds support for configuring the 3 built-in rate limiting policies via octopusdeploy_built_in_rate_limiting_policy.

Note that this is specifically for those three built-in policies which are all that exist in the initial implementation. If in future we were add support for custom rate limiting policies that could be deleted/created, they will be their own distinct resource type, because they're fundamentally different to these built-in ones which can only have a limited set of properties mutated and can never be created or deleted.

@rosslovas
rosslovas force-pushed the ross/add-built-in-rate-limiting-policies branch 2 times, most recently from 72960e6 to 3a5b0b2 Compare July 23, 2026 01:26
@rosslovas
rosslovas force-pushed the ross/add-built-in-rate-limiting-policies branch from 3a5b0b2 to b5d0bed Compare July 23, 2026 01:27
r.Config = ResourceConfiguration(req, resp)

if r.Config != nil {
resp.Diagnostics.Append(r.Config.EnsureResourceCompatibilityByVersion("built_in_rate_limiting_policy", "2026.3")...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I figured this is better than looking for the feature toggle, since the feature toggle won't stick around forever.

@rosslovas
rosslovas marked this pull request as ready for review July 23, 2026 01:51
Comment thread go.mod

require (
github.com/OctopusDeploy/go-octopusdeploy/v2 v2.113.1
github.com/OctopusDeploy/go-octopusdeploy/v2 v2.114.0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Bumped to pick up the prerequisite OctopusDeploy/go-octopusdeploy#428

@enf0rc3 enf0rc3 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good!

@IsaacCalligeros95 IsaacCalligeros95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, one minor comment around the delete method but I haven't dealt with a resource like this before where deleting is blocked and am basing this off Dom's prior work for a similar resource.

One other call out, I'm not sure if this requires a datasource. I don't think there's much value in it, I don't see where users would want to read this and reference properties elsewhere. The one benefit is if a user does want to do that they can just read the current server state and aren't forced into managing the state in terraform. Just a heads up, I think ignoring it makes more sense.

}

// Delete is a no-op as built-in rate limiting policies cannot be deleted.
func (r *builtInRateLimitingPolicyResource) Delete(_ context.Context, _ resource.DeleteRequest, _ *resource.DeleteResponse) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's some prior for resources that cannot be deleted here, it might be worth adding a warning.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great idea, I'll add something, thank you!

@rosslovas

Copy link
Copy Markdown
Contributor Author

One other call out, I'm not sure if this requires a datasource. I don't think there's much value in it, I don't see where users would want to read this and reference properties elsewhere. The one benefit is if a user does want to do that they can just read the current server state and aren't forced into managing the state in terraform. Just a heads up, I think ignoring it makes more sense.

Yeah agreed. I don't intend to add something right now but if the need ever comes up, I think it should be no problem to add later 👍

@rosslovas
rosslovas merged commit 9521a25 into main Jul 24, 2026
19 checks passed
@rosslovas
rosslovas deleted the ross/add-built-in-rate-limiting-policies branch July 24, 2026 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants