This repository was archived by the owner on Sep 4, 2026. It is now read-only.
Public release prep - #3
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are a few concrete doc/test issues (retry attempt count mismatch, baseURL wording, overly strict SemVer regex, minor wording) that should be corrected before release.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prepares the Firezone Go SDK for a first public release by hardening request construction (ID validation + path escaping), aligning update semantics with merge-patch behavior, and adding CI-grade conformance checks against the published OpenAPI spec.
Changes:
- Added centralized ID validation and safe request path building (
checkID/buildPath/resolvePath) and applied them across services. - Switched update calls to
PATCHand introducedNull[T]/ pointer-to-slice patterns to correctly express merge-patch “omit vs null vs set” semantics. - Added OpenAPI spec conformance tests (spec build tag + vendored spec) and expanded docs/templates/CI tasks for release readiness.
File summaries
| File | Description |
|---|---|
| user_agent_test.go | Adds tests for Version format and default/overridden User-Agent. |
| testdata/README.md | Documents vendored OpenAPI spec source and update workflow. |
| spec_test.go | Adds spec-tagged OpenAPI conformance checks for read models, request bodies, wrapper keys, and nullability rules. |
| sites.go | Validates IDs and uses escaped path building; switches update to PATCH. |
| sites_test.go | Updates expectations for PATCH on site updates. |
| SECURITY.md | Adds security reporting and scope guidance for the repository. |
| resources.go | Introduces merge-patch-safe nullable typing via *Null[T] and *[]T; validates IDs; switches update to PATCH. |
| resources_test.go | Adds update test ensuring PATCH and minimal merge body. |
| README.md | Expands user-facing docs: installation, retries, ID safety, merge-patch nullability, testing commands, and scope notes. |
| pool_members.go | Uses escaped base path and validates parent resource ID before calls. |
| policies.go | Updates merge-patch request typing (*Null, *[]T), validates IDs, switches update to PATCH, and checks IDs in enable/disable helpers. |
| policies_test.go | Updates expectations for PATCH in disable/enable flows. |
| path.go | Adds ErrMissingID, checkID, buildPath, and resolvePath for safe URL assembly. |
| path_test.go | Adds tests for escaping, nested paths, empty/dot IDs, base URL path prefixes, and base URL validation. |
| null.go | Adds Null[T] helper for “omit vs null vs set” in merge-patch update requests. |
| null_test.go | Verifies Null[T] JSON behavior and request bodies for update types. |
| mise.toml | Adds spec-check/spec-update tasks and improves acceptance-test task UX; includes spec vetting in CI-equivalent runs. |
| memberships.go | Uses escaped base path and validates group ID for nested membership endpoints. |
| internal/testutil/testutil.go | Adds NewClientWithOptions to support client option configuration in tests. |
| integration_test.go | Adds acceptance tests (integration build tag) verifying real portal behavior for merge-patch semantics, error envelopes, pagination, CRUD, and read-only resources. |
| groups.go | Validates IDs, uses escaped paths, switches update to PATCH. |
| groups_test.go | Adds group update test and asserts PATCH + expected body. |
| gateways.go | Documents token lifecycle decisions; validates IDs; uses escaped path building; switches update to PATCH. |
| gateways_test.go | Updates expectations for PATCH in gateway updates. |
| firezone.go | Introduces Version, default User-Agent with Go runtime, base URL validation, and switches request URL assembly to resolvePath. |
| example_test.go | Adds documentation examples for core usage patterns (validation errors, paging, merge-patch, retries, gateways). |
| errors.go | Clarifies IsConflict semantics in doc comment. |
| directories.go | Removes fields that don’t match spec and applies ID validation + escaped paths to directory gets. |
| CONTRIBUTING.md | Adds contributor guidance, conventions, and release checklist including spec checks and acceptance tests. |
| clients.go | Makes required update fields non-omitempty, validates IDs, uses escaped paths, switches update to PATCH. |
| clients_test.go | Updates expectations for PATCH in client updates. |
| CHANGELOG.md | Adds initial changelog and notes about PATCH semantics and scope. |
| auth_providers.go | Fixes nullable lifetime typing, adds ID validation, uses escaped paths for auth provider gets. |
| actors.go | Updates merge-patch nullable typing for email, validates IDs, uses escaped paths, switches update to PATCH. |
| actors_test.go | Updates expectations for PATCH in disable/enable flows. |
| .gitignore | Adds env file ignores. |
| .github/workflows/ci.yml | Runs spec-check in CI. |
| .github/PULL_REQUEST_TEMPLATE.md | Adds PR checklist including spec-check and acceptance tests. |
| .github/ISSUE_TEMPLATE/feature_request.md | Adds feature request template. |
| .github/ISSUE_TEMPLATE/config.yml | Adds issue template config with support/security contact links. |
| .github/ISSUE_TEMPLATE/bug_report.md | Adds bug report template. |
Review details
- Files reviewed: 40/42 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
181
to
+184
| Requests are retried automatically on HTTP 429 with exponential | ||
| backoff, honoring the API's `Retry-After` header (5 attempts by | ||
| default). Disable or tune this via `firezone.WithRetry`: | ||
| backoff, honoring the API's `Retry-After` header (10 attempts by | ||
| default). Only 429 is retried — network errors and 5xx responses are | ||
| returned to the caller, since neither is safe to assume idempotent. |
| // TestVersion pins the shape of Version. A version that isn't semver | ||
| // would break a consumer parsing it, and it ends up on the wire. | ||
| func TestVersion(t *testing.T) { | ||
| if !regexp.MustCompile(`^\d+\.\d+\.\d+(-[0-9A-Za-z.-]+)?$`).MatchString(firezone.Version) { |
Comment on lines
+45
to
+48
| - `Update` methods send `PATCH`. The API routes `PATCH` and `PUT` to the | ||
| same handler, but a partial update is what `PATCH` means, and sending | ||
| the matching verb insures against the two ever diverging. | ||
| `Memberships.ReplaceAll` and `PoolMembers.ReplaceAll` send `PUT`, |
Comment on lines
37
to
+40
| `baseURL` passed to `NewClient` is always the bare API host | ||
| (`https://api.firezone.dev`). | ||
| (`https://api.firezone.dev`). It must carry an `http`/`https` scheme and | ||
| a host, and no query or fragment — `NewClient` rejects anything else | ||
| rather than letting it surface later as a confusing transport error. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Multiple fixes and updates to prepare for public release.