Skip to content

feat: add rate limiting policy support#428

Open
rosslovas wants to merge 4 commits into
mainfrom
ross/add-rate-limiting-policies
Open

feat: add rate limiting policy support#428
rosslovas wants to merge 4 commits into
mainfrom
ross/add-rate-limiting-policies

Conversation

@rosslovas

@rosslovas rosslovas commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Adds support for an upcoming rate limiting feature, currently behind RateLimitingV2FeatureToggle. Three methods are added (pseudo-signatures:)

  • GetByID(GetRateLimitingPolicyByIdRequest) -> RateLimitingPolicy
  • List(ListRateLimitingPoliciesRequest) -> ListRateLimitingPoliciesResponse
  • Modify(ModifyRateLimitingPolicyCommand) -> RateLimitingPolicy

Id string `uri:"id"`
}

type ListRateLimitingPoliciesRequest struct {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not 100% sure I'm doing this the right way, as while there are some examples (e.g. live status) that are similar with requests and responses, there seem to be numerous different patterns at play in this repo. This seems the most strongly aligned with our target architecture though, and aligns with the C# client too.

@rosslovas
rosslovas force-pushed the ross/add-rate-limiting-policies branch 2 times, most recently from 68f0fbc to 24905ca Compare July 20, 2026 06:01
@rosslovas
rosslovas force-pushed the ross/add-rate-limiting-policies branch from 24905ca to 8f57a45 Compare July 20, 2026 06:16
@rosslovas
rosslovas marked this pull request as ready for review July 20, 2026 06:25
@borland

borland commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Review (from Claude)

I reviewed this PR for correctness, conformance with recent SDK patterns (using approvalpolicies from de61ff1 as the "good" reference), and alignment with the C# PR OctopusDeploy/OctopusDeploy#45175. Overall this is a clean, well-tested addition — go build, go vet, and the unit tests all pass, and the shape closely mirrors the C# client. A few things worth addressing before merge:

Alignment with the C# PR

  • AuditMode is present here but does not exist in the C# PR. Go adds AuditMode bool to both RateLimitingPolicy and ModifyRateLimitingPolicyCommand, whereas the C# RateLimitingPolicyResource / ModifyRateLimitingPolicyCommand have no such field. The e2e test round-trips AuditMode against a live server (modify → get), which suggests the server genuinely supports it and the C# PR is the incomplete one — but the two clients shouldn't silently diverge. Please confirm with the feature owner which is correct and get the other client updated to match.
  • NumberOfPages / LastPageNumber on ListRateLimitingPoliciesResponse exist here but not in the C# ListRateLimitingPoliciesResponse (which models only ItemType, TotalResults, ItemsPerPage, Items). Low risk since they're read-only response fields the e2e asserts against, but it's another modeling divergence worth reconciling.

Everything else aligns well: the core field set, the RateLimitingPolicyScopeType enum (same values/order — Unauthenticated, AuthenticatedHuman, AuthenticatedAgent), the request/command DTOs, the three operations (GetById/List/Modify → GET/GET/PUT), and the system-scoped /api/ratelimitingpolicies path (no space segment, matching C#'s IOctopusSystemRepository).

Conformance with SDK patterns

  • gofmt is not clean on two files — pkg/ratelimitingpolicies/rate_limiting_policy_service.go (single-line if err != nil { return ... } bodies) and test/e2e/rate_limiting_policy_service_test.go (blank lines inside the ModifyRateLimitingPolicyCommand literals break field alignment). The rest of the repo is gofmt-clean; please run gofmt -w over both.
  • Method naming: GetById — the SDK overwhelmingly uses GetByID (36 occurrences vs. 4 GetById), and Go's initialism convention prefers ID. Suggest renaming to GetByID for consistency.
  • Pattern choice (FYI, not a blocker): the newest sibling approvalpolicies uses the high-level helpers (newclient.GetByID/GetByQuery/Update, returning resources.Resources[T]). This PR instead calls low-level newclient.Get/Put with manual URI-template expansion. That's a reasonable choice here because the endpoint is system-scoped and uses bespoke request/response DTOs that intentionally mirror the C# PR — just flagging it's a lower-level style than the most recent addition. Not being wired into the Client struct is consistent with approvalpolicies (also functional-style).

Minor

  • ListRateLimitingPoliciesRequest.Skip uses uri:"skip,omitempty", so a zero-value request sends neither skip nor take, whereas C# always emits ?skip=&take=. The e2e ({} → 30/page) shows the server defaults gracefully, so this isn't a bug — just a behavioral difference from C#. (skip=0 can never be sent explicitly, but 0 is the default anyway, so it's harmless.)
  • Test-data nit: the unit test uses IDs like RateLimitingPolicies-2 (plural) while the e2e expects RateLimitingPolicy-1 (singular). Cosmetic only.

Nice work overall — the test coverage (marshal round-trips, invalid-enum, and end-to-end modify/error cases) is thorough.

@rosslovas

rosslovas commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator Author

The C# client is not merged because it was waiting for AuditMode which was only just added, and NumberOfPages etc. I didn't know existed until I was working on the Go version, and intend to bring it back to the C# one. Skip/take is a normal difference between the two clients AFAIK.

RateLimitingPolicies-1 vs RateLimitingPolicy-1: was waiting for the fix to land, I was required to use RateLimitingPolicy-1 at the time. I think the fix just merged so I can change it now. (Edit: it has, but the tests are gonna keep failing til the docker container updates so I'll have to wait)

gofmt: I ignored this because all of the code I looked at used one-liner if statements so I followed suit, TIL GoLand displays code differently to how it's actually written 🙃 Fixed!
image

I've changed GetById to GetByID but I'm not sure if I should change any other occurrence of Id given they're just replicas of the C# types, which use Id.

package ratelimitingpolicies

type RateLimitingPolicy struct {
Id string `json:"Id"`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Searching for json:"Id" finds that we are inconsistent, sometimes the Go field name is ID and other times Id.

ID seems to be winning on numbers though, and the Go style guide says "ID" so we should use that here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Will do. What about something like GetRateLimitingPolicyByIdRequest? Should I change that to GetRateLimitingPolicyByIDRequest? I wonder if we ever start code-genning these things, will we still try to do ID or allow it to remain Id to match the C# types 🤔

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

To help decide here, I did a case-sensitive find-in-files for ById and a second for ByID
There are 44 ById and 968 ByID.

Combined with the google style guide saying ID, I think that's a pretty clear push to GetRateLimitingPolicyByIDRequest

type RateLimitingPolicyScopeType int

const (
Unauthenticated RateLimitingPolicyScopeType = iota

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just checking, this serializes across the JSON API as a string, doesn't it?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yep! TestRateLimitingPolicyScopeTypeJsonMarshal goes back and forth to confirm

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

TIL about this enumer code generator

@borland borland left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Apart from the casing of ID, LGTM. Approved so you can merge when you're ready

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.

2 participants