Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support vetting source available license #38

Merged
merged 4 commits into from
Oct 16, 2023

Conversation

lieut-data
Copy link
Member

Summary

This change is a prerequisite to introducing the new, Source Available enterprise package within the monorepo, scheduled to contain a (small) subset of the current enterprise functionality.

We currently run go vet with our custom vet tool with two invocations, essentially vetting the server/... package within the monorepo using -license, and then the separate enterprise/... repository using -enterpriseLicense:

	@VET_CMD="-license -structuredLogging -inconsistentReceiverName -inconsistentReceiverName.ignore=session_serial_gen.go,team_member_serial_gen.go,user_serial_gen.go -emptyStrCmp -tFatal -configtelemetry -errorAssertions"; \
	$(GO) vet -vettool=$(GOBIN)/mattermost-govet $$VET_CMD ./...
ifeq ($(BUILD_ENTERPRISE_READY),true)
  ifneq ($(MM_NO_ENTERPRISE_LINT),true)
	$(GO) vet -vettool=$(GOBIN)/mattermost-govet -enterpriseLicense -structuredLogging -tFatal $(BUILD_ENTERPRISE_DIR)/...
  endif
endif

This pull request proposes automatically detecting the new enterprise package at github.com/mattermost/mattermost/server/v8/enterprise and treating it slightly differently for the purpose of vetting the license. In principle, we could support a -sourceAvailable license check, but then we'd need a way to ./... while excluding the enterprise/ package.

The downside to this approach is that we're effectively embedding the major version into this tool, and it will break if we ever bump to v9/. (Though with the new public submodule, it's not really clear we ever "need" to do that.)

Suggestions welcome on better approaches here! Once this is merged, though, I'll push the affected enterprise source to the monorepo.

Ticket Link

None.

Automatically detect the new, [Source Available](https://docs.mattermost.com/about/faq-mattermost-source-available-license.html). `enterprise` package within the existing `github.com/mattermost/mattermost/server/v8` module and ensure the appropriate comment.

Path checking is preferred to a new, `sourceAvailableLicense` command since we want to retain the ability to `go vet ./...` within the parent `server` package. This does mean it's slightly brittle to major version changes (e.g. switching to v9).
@lieut-data lieut-data added the 2: Dev Review Requires review by a core committer label Oct 10, 2023
@lieut-data lieut-data added Do Not Merge Should not be merged until this label is removed and removed Do Not Merge Should not be merged until this label is removed labels Oct 13, 2023
@agnivade
Copy link
Member

The downside to this approach is that we're effectively embedding the major version into this tool, and it will break if we ever bump to v9/. (Though with the new public submodule, it's not really clear we ever "need" to do that.)

We should bump it to v9 to match with the release version. It's a mistake we haven't done that. @lieut-data - would you like to open a PR to bump that? I'll also check with Amy to add this as a reminder in the checklist for every major version release.

For this PR, could we use a regex to match anything >=8?

@lieut-data
Copy link
Member Author

lieut-data commented Oct 16, 2023

Using a regex is brilliant! Thanks @agnivade.

On bumping v8/: is there any benefit from doing so? I'm imagining a future where everything we intend to share is in the public submodule and everything else is under internal/. (I realize we can't do that now because enterprise…)

But in that future, do we care about the major version at all anymore? And even now for those clients that still have to import things outside of the public submodule, we almost always recommend pinning at a hash anyway.

Thoughts on just leaving it?

@lieut-data lieut-data removed the request for review from jespino October 16, 2023 14:03
@lieut-data
Copy link
Member Author

/update-branch

@lieut-data lieut-data merged commit 2703fdd into new Oct 16, 2023
5 checks passed
@lieut-data lieut-data deleted the vet-source-available-enterprise branch October 16, 2023 14:45
@hanzei hanzei added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants