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

MM-55060 - Calls: Add PushTypeCalls for calls-specific push notifications #110

Merged
merged 7 commits into from
Nov 15, 2023

Conversation

cpoile
Copy link
Member

@cpoile cpoile commented Nov 10, 2023

Summary

Ticket Link

Copy link

Unit Test Results

17 tests  ±0   17 ✔️ ±0   16s ⏱️ ±0s
  3 suites ±0     0 💤 ±0 
  1 files   ±0     0 ±0 

Results for commit f45900b. ± Comparison against base commit d41f502.

Makefile Outdated
@@ -23,7 +25,7 @@ ifeq ($(DIFF), 1)
endif

PP_PKG=github.com/mattermost/mattermost-push-proxy/internal/version
LDFLAGS="-X $(PP_PKG).gitVersion=$(GIT_VERSION) -X $(PP_PKG).gitCommit=$(GIT_HASH) -X $(PP_PKG).gitTreeState=$(GIT_TREESTATE) -X $(PP_PKG).buildDate=$(BUILD_DATE)"
LDFLAGS="-X $(PP_PKG).gitVersion=$(GIT_VERSION) -X $(PP_PKG).buildHash=$(BUILD_HASH) -X $(PP_PKG).buildTagLatest=$(BUILD_TAG_LATEST) -X $(PP_PKG).buildTagCurrent=$(BUILD_TAG_CURRENT) -X $(PP_PKG).gitTreeState=$(GIT_TREESTATE) -X $(PP_PKG).buildDate=$(BUILD_DATE)"
Copy link
Member

Choose a reason for hiding this comment

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

Heads up that there are some changes to the Makefile here: #109.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, merged and updated the Makefile.

server/server.go Show resolved Hide resolved
@cpoile cpoile requested a review from agnivade November 13, 2023 17:57
internal/version/version.go Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
Copy link

@streamer45 streamer45 left a comment

Choose a reason for hiding this comment

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

Nicely done!

@enahum
Copy link
Contributor

enahum commented Nov 14, 2023

This approach solves the new plugin + new proxy + new app, but what about the other cases?

For example what if the app is not up to date?

Perhaps we should have a quick convo about this if you want me to clarify.

In my mind, regardless of the combination of elements: server, proxy, plugin, app. The notification should work as was already introduced. Backwards compatibility needs to be kept to avoid bug reports

@cpoile
Copy link
Member Author

cpoile commented Nov 14, 2023

@enahum Excellent point, overlooked that. Fixed (see: mattermost/mattermost#25405 (comment) ) for an explanation. Thank you!

@enahum
Copy link
Contributor

enahum commented Nov 15, 2023

@cpoile thanks for the change.. now it seems to take care of the backwards compatibility

@streamer45
Copy link

@cpoile Is the /version endpoint used anywhere after the recent changes? I am okay with keeping it but I suppose it's not strictly needed any longer?

@cpoile
Copy link
Member Author

cpoile commented Nov 15, 2023

@streamer45 Nope, but I did want to keep it so that it's there in previous versions if we ever do need it in the future.

@streamer45 streamer45 removed the 1: Dev Review Requires review by a core commiter label Nov 15, 2023
@streamer45 streamer45 added the 2: Reviews Complete All reviewers have approved the pull request label Nov 15, 2023
@cpoile cpoile merged commit d0c02f2 into master Nov 15, 2023
6 checks passed
@cpoile cpoile deleted the MM-55060-calls-type-and-version branch November 15, 2023 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants