feat: add group v2 support #132
Conversation
sorccu
requested review from
maxigimenez,
pilimartinez,
Antoine-C and
shiini2
as code owners
|
""" WalkthroughThe changes introduce API versioning support in the client by refactoring HTTP API call methods to use explicit versioned wrappers ( Changes
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
types.go (1)
822-831: Strict string comparison may reject valid JSON with insignificant whitespace.
bytes.Equal(data, []byte("FALLBACK"))only passes for the exact byte sequence"FALLBACK". Inputs like" FALLBACK"or"\n\"FALLBACK\" "are valid JSON but fail the check. Consider decoding to a string first or trimming surrounding whitespace before the comparison.types_test.go (1)
441-446: Mark helper to improve failure location.
testBytesEqualshould callt.Helper()so that failures are reported at the caller, not inside the helper itself.func testBytesEqual(t *testing.T, got, want []byte) { - if !bytes.Equal(got, want) { - t.Fatalf("expected: %s, got: %s", want, got) - } + t.Helper() + if !bytes.Equal(got, want) { + t.Fatalf("expected: %s, got: %s", want, got) + } }checkly.go (1)
1863-1864: Usepath.Join(or sanitize) to avoid double slashes in the request URL.
fmt.Sprintf("%s/v%d/%s", c.url, version, URL)will emit//ifc.urlalready ends with/or ifURLis prefixed with/. A safer approach is:requestURL := fmt.Sprintf("%s/v%d/%s", strings.TrimRight(c.url, "/"), version, strings.TrimLeft(URL, "/"))or
path.Joinfor clarity.This avoids subtle 301/308 redirects and mismatched signature checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
checkly.go(63 hunks)types.go(7 hunks)types_test.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
types_test.go (1)
types.go (1)
RetryStrategy(814-820)
checkly.go (1)
types.go (2)
GroupV2(901-924)Group(870-898)
sorccu
force-pushed
the
simokinnunen/sc-24521/tf-groups-v2-support
branch
from
13a5072 to
a4c1309
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
types.go (1)
134-140: ExposeCreateGroupV2but consider parity helpers.Introducing a V2 creation method is great, but the interface is starting to bloat.
If V1 is going away soon, think about embedding a dedicatedGroupServiceV2(or similar) instead of piling more methods onto the main interface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
checkly.go(63 hunks)types.go(7 hunks)types_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- types_test.go
- checkly.go
🔇 Additional comments (7)
types.go (7)
4-4:bytesimport looks correct.The additional import is required for the
bytes.Equalcomparison introduced later in the file – no issues spotted.
148-154: Method naming is consistent – good job.
GetGroupV2mirrors the existing V1 signature, keeping cognitive load low.
164-172: Consistency check: missingDeleteGroupV2.You added create/get/update for V2 but not delete. Deliberate? If the endpoint exists, omitting it forces callers to mix versions.
772-779: Pointer fields correctly convey “optional”.Using pointers inside
AlertSettingsV2avoids accidental zero-value overrides – nice.
853-867:MarshalJSONpath is symmetric – all good.The fallback-as-string encoding matches the unmarshal logic.
900-924:GroupV2design is flexible but double-check required fields.Using pointers for every field lets callers perform partial updates, yet it also means forgetting to set
Name(still non-pointer) will yield confusing 422s from the API. Ensure upstream validators or builders enforce the minimal set.
936-944:APICheckDefaultsV2mirrors V1 cleanly – nice extraction.Pointer slices keep zero-value vs “explicit empty” semantics intact.
types.go
Outdated
| func (s *RetryStrategy) UnmarshalJSON(data []byte) error { | ||
| // This is a special value for a group, indicating that all checks in | ||
| // the group should use their own retry strategies. For the Go API we | ||
| // just pretend it's an actual retry strategy type. | ||
| if bytes.Equal(data, []byte(`"FALLBACK"`)) { | ||
| *s = RetryStrategy{ | ||
| Type: "FALLBACK", | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| type Underlying RetryStrategy | ||
|
|
||
| var underlying *Underlying | ||
|
|
||
| if err := json.Unmarshal(data, &underlying); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| *s = RetryStrategy{ | ||
| Type: underlying.Type, | ||
| BaseBackoffSeconds: underlying.BaseBackoffSeconds, | ||
| MaxRetries: underlying.MaxRetries, | ||
| MaxDurationSeconds: underlying.MaxDurationSeconds, | ||
| SameRegion: underlying.SameRegion, | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against null and simplify unmarshal logic.
A null JSON payload will leave underlying == nil, triggering a panic when you copy its fields.
You can also avoid the double-pointer dance:
func (s *RetryStrategy) UnmarshalJSON(data []byte) error {
// special string case
if bytes.Equal(data, []byte(`"FALLBACK"`)) {
*s = RetryStrategy{Type: "FALLBACK"}
return nil
}
+
+ // treat explicit null as zero-value
+ if bytes.Equal(bytes.TrimSpace(data), []byte("null")) {
+ *s = RetryStrategy{}
+ return nil
+ }
- type Underlying RetryStrategy
- var underlying *Underlying
- if err := json.Unmarshal(data, &underlying); err != nil {
- return err
- }
- *s = RetryStrategy{
- Type: underlying.Type,
- BaseBackoffSeconds: underlying.BaseBackoffSeconds,
- MaxRetries: underlying.MaxRetries,
- MaxDurationSeconds: underlying.MaxDurationSeconds,
- SameRegion: underlying.SameRegion,
- }
+ type alias RetryStrategy
+ var tmp alias
+ if err := json.Unmarshal(data, &tmp); err != nil {
+ return err
+ }
+ *s = RetryStrategy(tmp)
return nil
}This removes a potential nil-pointer panic and halves the code.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *RetryStrategy) UnmarshalJSON(data []byte) error { | |
| // This is a special value for a group, indicating that all checks in | |
| // the group should use their own retry strategies. For the Go API we | |
| // just pretend it's an actual retry strategy type. | |
| if bytes.Equal(data, []byte(`"FALLBACK"`)) { | |
| *s = RetryStrategy{ | |
| Type: "FALLBACK", | |
| } | |
| return nil | |
| } | |
| type Underlying RetryStrategy | |
| var underlying *Underlying | |
| if err := json.Unmarshal(data, &underlying); err != nil { | |
| return err | |
| } | |
| *s = RetryStrategy{ | |
| Type: underlying.Type, | |
| BaseBackoffSeconds: underlying.BaseBackoffSeconds, | |
| MaxRetries: underlying.MaxRetries, | |
| MaxDurationSeconds: underlying.MaxDurationSeconds, | |
| SameRegion: underlying.SameRegion, | |
| } | |
| return nil | |
| } | |
| func (s *RetryStrategy) UnmarshalJSON(data []byte) error { | |
| // This is a special value for a group, indicating that all checks in | |
| // the group should use their own retry strategies. For the Go API we | |
| // just pretend it's an actual retry strategy type. | |
| if bytes.Equal(data, []byte(`"FALLBACK"`)) { | |
| *s = RetryStrategy{ | |
| Type: "FALLBACK", | |
| } | |
| return nil | |
| } | |
| // treat explicit null as zero-value | |
| if bytes.Equal(bytes.TrimSpace(data), []byte("null")) { | |
| *s = RetryStrategy{} | |
| return nil | |
| } | |
| type alias RetryStrategy | |
| var tmp alias | |
| if err := json.Unmarshal(data, &tmp); err != nil { | |
| return err | |
| } | |
| *s = RetryStrategy(tmp) | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In types.go around lines 822 to 851, the UnmarshalJSON method for RetryStrategy
does not guard against a null JSON payload, which can cause a nil-pointer panic
when accessing underlying fields. To fix this, change the unmarshalling target
from a pointer to a value type to avoid the double-pointer usage, and add a
check to handle null data by returning early or setting a default value. This
simplifies the code and prevents panics by ensuring underlying is never nil
before accessing its fields.
a4c1309 to
95ca8e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
types.go (1)
822-851: Simplify and hardenRetryStrategy.UnmarshalJSON
- Struct-literal copy is unnecessary; you can cast the alias back to the real type:
- *s = RetryStrategy{ - Type: underlying.Type, - BaseBackoffSeconds: underlying.BaseBackoffSeconds, - MaxRetries: underlying.MaxRetries, - MaxDurationSeconds: underlying.MaxDurationSeconds, - SameRegion: underlying.SameRegion, - } + *s = RetryStrategy(underlying)
- A
"null"payload still triggersjson.Unmarshaland leavesunderlyingzero-valued, but the API currently panics on an explicitnull. Guard for it up-front:+ if bytes.Equal(bytes.TrimSpace(data), []byte("null")) { + *s = RetryStrategy{} + return nil + }Result: shorter code, nil-safety, and fixed gosimple S1016 warning.
🧹 Nitpick comments (2)
checkly.go (2)
1856-1864: Potential double “/” in constructed URLs
requestURL := fmt.Sprintf("%s/v%d/%s", c.url, version, URL)
IfbaseURLalready ends with/, the final URL contains//v1/…. Although most servers accept this, it’s best avoided:- requestURL := fmt.Sprintf("%s/v%d/%s", c.url, version, URL) + requestURL := fmt.Sprintf("%s/v%d/%s", + strings.TrimRight(c.url, "/"), + version, + strings.TrimLeft(URL, "/"))
606-635: Allocate empty slices once to avoid escaped heap allocationsIn
UpdateGroupV2you build an anonymous slice each time:group.Locations = &[]string{}Reuse a package-level var to avoid repeated allocations:
var emptyStringSlice = []string{} if group.Locations == nil { group.Locations = &emptyStringSlice }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
checkly.go(63 hunks)types.go(7 hunks)types_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- types_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
checkly.go (1)
types.go (2)
GroupV2(901-924)Group(870-898)
🪛 golangci-lint (1.64.8)
types.go
842-842: S1016: should convert underlying (type Underlying) to RetryStrategy instead of using struct literal
(gosimple)
🔇 Additional comments (3)
types.go (1)
900-924:GroupV2still forcesNameon PATCH-like updatesAll other fields are pointers to distinguish “unset” vs. “zero”, but
Name(andID) are values, making them mandatory on everyUpdateGroupV2call.
Double-check the backend: if it allows partial updates, consider changing to*string(and*int64forID) to align with the optional semantics you introduced for the rest of the struct.checkly.go (2)
1838-1855: Nice abstraction for versioned endpointsThe thin
apiCallV1/apiCallV2wrappers read well and keep call sites clean. 👍
822-851: ```shell
#!/bin/bashEnsure no remaining direct calls to apiCall without the version wrappers.
rg --line-number '.apiCall(' | grep -v 'func (c *client) apiCall' | grep -v 'apiCallV[12]'
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Tests to follow
Affected Components
Style
go fmtNotes for the Reviewer
New Dependency Submission