Skip to content

chore(license): rename temporary -> trial and customer -> enterprise#325

Merged
iamjoemccormick merged 3 commits into
mainfrom
falk/rename-license-types
Jun 16, 2026
Merged

chore(license): rename temporary -> trial and customer -> enterprise#325
iamjoemccormick merged 3 commits into
mainfrom
falk/rename-license-types

Conversation

@philippfalk

@philippfalk philippfalk commented May 6, 2026

Copy link
Copy Markdown
Member

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:

  • Documentation:
    • Does developer documentation (code comments, readme, etc.) need to be added or updated?
    • Does the user documentation need to be expanded or updated for this change?
  • Testing:
    • Does this functionality require changing or adding new unit tests?
    • Does this functionality require changing or adding new integration tests?
  • Git Hygiene:

For more details refer to the Go coding standards and the pull request process.

@philippfalk
philippfalk requested a review from a team as a code owner May 6, 2026 12:35
@philippfalk
philippfalk force-pushed the falk/rename-license-types branch from fd8c9c3 to 7ce1ec7 Compare May 6, 2026 12:41
@philippfalk philippfalk self-assigned this May 6, 2026
@philippfalk philippfalk added refactoring Issues related to refactoring existing code for maintainability or readablity. ctl Issues primarily affecting the BeeGFS control tool. labels May 6, 2026

@iamjoemccormick iamjoemccormick left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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) and CERT_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 and beegfs license --json output (which already emits numeric enum values via encoding/json).
  • No leftover references. A repo-wide grep for CERT_TYPE_CUSTOMER and CERT_TYPE_TEMPORARY returns no hits; the rename in beegfs-go is exhaustive for the source surface.
  • User-visible string change. beegfs license output now reads "BeeGFS Trial License Certificate" and "WARNING: This system has a trial license installed." in place of the "Temporary" wording. There is no CHANGELOG/RELEASE_NOTES file 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 in beegfs-rust/ will need an equivalent consumer-side update + Cargo.toml bump when the protobuf rename is tagged.

@philippfalk

Copy link
Copy Markdown
Member Author

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.

Fixed after merging protobuf.

🟡 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.

Fixed.

🟣 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.

Good catch. Fixed in latest commit by using getters.

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.

Not worth fixing in my opinion. Callers have to make sure not to pass in nil.

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.

Not a problem in my opinion. More unlikely errors are ignored in that code path.

@philippfalk
philippfalk force-pushed the falk/rename-license-types branch from 92fc39e to 606a909 Compare May 12, 2026 11:00

@iamjoemccormick iamjoemccormick left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM - don't forget to squash.

@iamjoemccormick iamjoemccormick left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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!

@iamjoemccormick

iamjoemccormick commented Jun 16, 2026

Copy link
Copy Markdown
Member

Force push to rebase on main and resolve merge conflict in go.mod and go.sum. Here is the range diff confirming nothing changed in this branch after resolving the conflicts:

$ git range-diff origin/main...origin/falk/rename-license-types origin/main...falk/rename-license-types
1:  6b40cee = 1:  42ff99f chore(license): rename temporary -> trial and customer -> enterprise
2:  fd00421 = 2:  a9251e8 fix(review): address comments
3:  134b9b0 < -:  ------- chore: bump protobuf dependency
4:  eb4ad7b < -:  ------- feat(telemetry): Add OpenTelemetry metrics and log export to beegfs-go (#314)
-:  ------- > 3:  82c27cb chore: bump protobuf dependency

Bypassing branch protection rules to get this merged since nothing has actually changed since my last review.

@iamjoemccormick
iamjoemccormick merged commit adb37dc into main Jun 16, 2026
7 checks passed
@iamjoemccormick
iamjoemccormick deleted the falk/rename-license-types branch June 16, 2026 22:43
@iamjoemccormick iamjoemccormick mentioned this pull request Jun 16, 2026
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ctl Issues primarily affecting the BeeGFS control tool. refactoring Issues related to refactoring existing code for maintainability or readablity.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants