-
Notifications
You must be signed in to change notification settings - Fork 90
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
New secret rudder #232
New secret rudder #232
Conversation
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #232 +/- ##
=======================================
Coverage 43.06% 43.06%
=======================================
Files 7 7
Lines 1038 1038
=======================================
Hits 447 447
Misses 566 566
Partials 25 25 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few non-blocking comments
build/custom.mk
Outdated
@@ -19,4 +25,4 @@ ifndef MM_RUDDER_WRITE_KEY | |||
MM_RUDDER_WRITE_KEY = 1d5bMvdrfWClLxgK1FvV3s4U1tg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems there's a remnant reference to MM_RUDDER_WRITE_KEY
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch!
@@ -19,4 +25,4 @@ ifndef MM_RUDDER_WRITE_KEY | |||
MM_RUDDER_WRITE_KEY = 1d5bMvdrfWClLxgK1FvV3s4U1tg | |||
endif | |||
|
|||
GO_BUILD_FLAGS += -ldflags '-X "github.com/mattermost/mattermost-plugin-api/experimental/telemetry.rudderWriteKey=$(MM_RUDDER_WRITE_KEY)"' | |||
GO_BUILD_FLAGS += -ldflags '-X "github.com/mattermost/mattermost-plugin-api/experimental/telemetry.rudderWriteKey=$(RUDDER_WRITE_KEY)"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have the definition of RUDDER_WRITE_KEY
be above this line, rather than at the top of the file?
cd server && env CGO_ENABLED=0 GOOS=linux GOARCH=amd64 $(GO) build $(GO_BUILD_FLAGS) $(GO_BUILD_GCFLAGS) -trimpath -o dist/plugin-linux-amd64; | ||
cd server && env CGO_ENABLED=0 GOOS=darwin GOARCH=amd64 $(GO) build $(GO_BUILD_FLAGS) $(GO_BUILD_GCFLAGS) -trimpath -o dist/plugin-darwin-amd64; | ||
cd server && env CGO_ENABLED=0 GOOS=windows GOARCH=amd64 $(GO) build $(GO_BUILD_FLAGS) $(GO_BUILD_GCFLAGS) -trimpath -o dist/plugin-windows-amd64.exe; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably for another PR, but we should add builds for darwin-arm64
and linux-arm64
here and in plugin.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same issue with todo plugin, I did it and then noticed that it doesn't build due to server/v6 being required and reverted it.
Not sure yet if that's easy to migrate but I didn't want to go that far in this telemetry PR.
Summary
After some name changing, MM_RUDDER_WRITE_KEY has become MM_RUDDER_PLUGINS_PROD.
Fix custom.mk so we get the right data.
I tried to add arm64 architectures but it implies update to mattermost-server/v6 so I removed that from the PR. I kept the refactor for
make server
tho.Ticket Link
no