Description
The Go SDK has 14 functional option types applied at 16 call sites across six
packages. Only one of them, ApplyCreateOptions, guards against a nil option.
Every other apply loop calls opt(&cfg) unconditionally and panics with a nil
pointer dereference if a caller passes a nil option.
Two things should be decided and then applied consistently:
-
Nil policy. Either ignore nil options everywhere, or document that a nil
option is a programming error and let it panic. Right now the SDK does one
thing in Create and the opposite everywhere else, which is the worst of
both options because callers cannot rely on either behavior.
-
Apply-loop duplication. Six Apply*Options helpers live in types/,
while oidc, fake, gateway, edge, and the v1 transport clients each
hand-roll the same loop inline. A single generic helper would collapse all of
them and make the nil policy a one-line decision instead of a 16-site edit.
Context
This came out of review on #3198, which replaced the positional labels
parameter on Create() and CreateFromTemplate() with functional options.
Because the removed parameter sat immediately before the variadic, existing
calls like Create(ctx, ws, name, spec, nil) still compiled and began panicking
at runtime. That was fixed in #3198 by nil-guarding ApplyCreateOptions only,
since it was the one type with an actual migration hazard.
The remaining types have no such hazard, so there is no urgency here. This is a
consistency and API-design question rather than a bug.
Current state
| Location |
Applier |
Nil-guarded |
types/options.go:34 |
ApplyCreateOptions |
yes |
types/log.go:72 |
ApplyLogOptions |
no |
types/policy.go:267,304,348,400 |
ApplyGetDraftOptions, ApplyApproveAllOptions, ApplyGetStatusOptions, ApplyListPolicyOptions |
no |
oidc/oidc.go:36, oidc/device.go:49, oidc/credentials.go:40 |
inline |
no |
fake/fake.go:100 |
inline |
no |
gateway/gateway.go:30 |
inline |
no |
ssh_client.go:67, tcp_client.go:46,106 |
inline |
no |
edge/tunnel.go:110, auth_refresh.go:163 |
inline |
no |
Definition of Done
Description
The Go SDK has 14 functional option types applied at 16 call sites across six
packages. Only one of them,
ApplyCreateOptions, guards against a nil option.Every other apply loop calls
opt(&cfg)unconditionally and panics with a nilpointer dereference if a caller passes a nil option.
Two things should be decided and then applied consistently:
Nil policy. Either ignore nil options everywhere, or document that a nil
option is a programming error and let it panic. Right now the SDK does one
thing in
Createand the opposite everywhere else, which is the worst ofboth options because callers cannot rely on either behavior.
Apply-loop duplication. Six
Apply*Optionshelpers live intypes/,while
oidc,fake,gateway,edge, and thev1transport clients eachhand-roll the same loop inline. A single generic helper would collapse all of
them and make the nil policy a one-line decision instead of a 16-site edit.
Context
This came out of review on #3198, which replaced the positional
labelsparameter on
Create()andCreateFromTemplate()with functional options.Because the removed parameter sat immediately before the variadic, existing
calls like
Create(ctx, ws, name, spec, nil)still compiled and began panickingat runtime. That was fixed in #3198 by nil-guarding
ApplyCreateOptionsonly,since it was the one type with an actual migration hazard.
The remaining types have no such hazard, so there is no urgency here. This is a
consistency and API-design question rather than a bug.
Current state
types/options.go:34ApplyCreateOptionstypes/log.go:72ApplyLogOptionstypes/policy.go:267,304,348,400ApplyGetDraftOptions,ApplyApproveAllOptions,ApplyGetStatusOptions,ApplyListPolicyOptionsoidc/oidc.go:36,oidc/device.go:49,oidc/credentials.go:40fake/fake.go:100gateway/gateway.go:30ssh_client.go:67,tcp_client.go:46,106edge/tunnel.go:110,auth_refresh.go:163Definition of Done
explains why per-package loops are kept