chore(license): rename temporary -> trial and customer -> enterprise#325
Conversation
fd8c9c3 to
7ce1ec7
Compare
iamjoemccormick
left a comment
There was a problem hiding this comment.
LGTM. Also ran Claude against it which only flagged that we need to merge the protobuf change and update go.mod here before merging (our build checks prevent that anyway).
Pasting Claude's nit and pre-existing findings in case you want to take action on any of those in this PR.
Claude's Review (Opus 4.7)
Review summary
Reviewing falk/rename-license-types in beegfs-go — author: 80761729+philippfalk@users.noreply.github.com
1 important · 1 nit · 3 pre-existing
Source-level rename is correct and complete, but the branch as-pushed is not mergeable because it depends on an unreleased version of the protobuf module that the branch's go.mod does not pin.
🔴 Important findings
Branch is not buildable against any published protobuf release beegfs-go/go.mod:32
The protobuf rename commit (5d14ce8) lives on protobuf's own unmerged falk/rename-license-types branch — it is not on protobuf main and is not covered by any tag. The latest protobuf tag is v0.8.3 (commit a80b997), which still has CERT_TYPE_CUSTOMER / CERT_TYPE_TEMPORARY.
beegfs-go's go.mod pins github.com/thinkparq/protobuf v0.8.3-0.20260212175550-a80b99765f45 — that pseudo-version resolves to the pre-rename a80b997. The branch as-pushed has no go.mod bump and no replace directive (your local uncommitted replace github.com/thinkparq/protobuf => ../protobuf is what's letting your machine compile it).
Before this merges, the protobuf side has to land first (merge + new tag, presumably v0.8.4), and then this branch needs a go.mod bump to that tag. Otherwise CI and external consumers will fail with undefined: pl.CertType_CERT_TYPE_ENTERPRISE.
🟡 Nit findings
Stale "temp" wording in renamed branch's comment beegfs-go/ctl/pkg/ctl/license/license.go:71
The comment reads // For temp licenses only warn 14 days before. but the conditional one line above is now CertType_CERT_TYPE_TRIAL. The corresponding sites in ctl/internal/cmd/license/license.go updated their comments and user-facing strings to "trial"; this one was missed. Suggest: // For trial licenses only warn 14 days before.
🟣 Pre-existing issues
Nil-deref on license.Data.ParentData in CLI output beegfs-go/ctl/internal/cmd/license/license.go:228-230
runLicenseCmd reads license.Data.ParentData.Organization, .Locality, .Country directly. ParentData is an optional CertData in protobuf/proto/license.proto:91. If mgmtd ever returns a CertData without ParentData populated (e.g., self-signed test cert, partial response, future code path), the CLI will panic instead of printing a license summary. Not introduced by this PR.
Check() and GetTimeToExpiration() assume non-nil license.Data beegfs-go/ctl/pkg/ctl/license/license.go:54-70, 163-166
Check calls GetLicense, then dereferences license.Result, license.Data.Type, license.Data.DnsNames, and passes license to GetTimeToExpiration which reads license.Data.ValidUntil.AsTime(). None of these guard against license == nil or license.Data == nil. The CLI entry point at ctl/internal/cmd/license/license.go:128 does check license == nil, but the public-API path through Check is exposed to library consumers without that guard. Not introduced by this PR.
Discarded JSON marshal errors in --json output beegfs-go/ctl/internal/cmd/license/license.go:140,143
Both json.MarshalIndent(license, …) and json.Marshal(license) discard their error with _. If marshaling ever fails (unlikely for a well-formed proto message, but not impossible — e.g., NaN/Inf in a future field), the CLI will print an empty line and silently exit success. Worth wrapping in an error path. Not introduced by this PR.
Notes
- Wire compatibility is preserved.
CERT_TYPE_ENTERPRISE = 4(was_CUSTOMER) andCERT_TYPE_TRIAL = 5(was_TEMPORARY) keep their wire numbers, so a CLI on the old protobuf and a mgmtd on the new one (or vice versa) still interoperate at the gRPC level. No JSON/textproto serializer in this repo emits the enum name, so renaming the names is safe across both wire andbeegfs license --jsonoutput (which already emits numeric enum values viaencoding/json). - No leftover references. A repo-wide grep for
CERT_TYPE_CUSTOMERandCERT_TYPE_TEMPORARYreturns no hits; the rename in beegfs-go is exhaustive for the source surface. - User-visible string change.
beegfs licenseoutput now reads "BeeGFS Trial License Certificate" and "WARNING: This system has a trial license installed." in place of the "Temporary" wording. There is noCHANGELOG/RELEASE_NOTESfile in this repo to update, so this is a release-notes item for whoever cuts the next binary release rather than a repo change. - mgmtd side (Rust) not in scope. This review covered only
beegfs-go. The Rust mgmtd inbeegfs-rust/will need an equivalent consumer-side update +Cargo.tomlbump when the protobuf rename is tagged.
Fixed after merging protobuf.
Fixed.
Good catch. Fixed in latest commit by using getters.
Not worth fixing in my opinion. Callers have to make sure not to pass in
Not a problem in my opinion. More unlikely errors are ignored in that code path. |
92fc39e to
606a909
Compare
iamjoemccormick
left a comment
There was a problem hiding this comment.
LGTM - don't forget to squash.
606a909 to
134b9b0
Compare
iamjoemccormick
left a comment
There was a problem hiding this comment.
LGTM provided the only conflicts you had to resolve in the force push were in generated files (go.mod and go.sum). Don't forget to squash!
134b9b0 to
82c27cb
Compare
|
Force push to rebase on main and resolve merge conflict in Bypassing branch protection rules to get this merged since nothing has actually changed since my last review. |
What does this PR do / why do we need it?
Renames two license types to be more accurate regarding the purpose they actually serves. This goes hand-in-hand with a PR in ThinkParQ/protobuf.
Related Issue(s)
None
Where should the reviewer(s) start reviewing this?
Relates to ThinkParQ/protobuf#74, but changes are trivial. Will only build once the protobuf PR is merged and we update the dependency.
Are there any specific topics we should discuss before merging?
No
What are the next steps after this PR?
None
Checklist before merging:
Required for all PRs.
When creating a PR these are items to keep in mind that cannot be checked by GitHub actions:
For more details refer to the Go coding standards and the pull request process.