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.

Sign up for GitHub

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

Jump to bottom

[MM-54589] Implement job status API #542

Merged
merged 5 commits into from
Oct 5, 2023
Merged

[MM-54589] Implement job status API #542

merged 5 commits into from
Oct 5, 2023

Conversation

streamer45
Copy link
Collaborator

Summary

PR adds a new bot API endpoint to explicitly report job statuses (e.g. failure, started).

Of note, we are now publishing a github.com/mattermost/mattermost-plugin-calls/server/public Go module with shared types. I'll slowly start to port over some common structures as needed (e.g. some of those defined in mattermost/rtcd#115). This is still subject to breaking changes and should only be used internally for the time being.

Related PR

mattermost/calls-recorder#46

Ticket Link

https://mattermost.atlassian.net/browse/MM-54589

@streamer45 streamer45 added the 2: Dev Review Requires review by a core committer label Oct 2, 2023
@streamer45 streamer45 added this to the v0.21.0 milestone Oct 2, 2023
@streamer45 streamer45 requested a review from cpoile October 2, 2023 22:28
@streamer45 streamer45 self-assigned this Oct 2, 2023
@streamer45 streamer45 mentioned this pull request Oct 2, 2023
Merged
streamer45
streamer45 commented Oct 2, 2023
View reviewed changes
go.mod
@@ -29,6 +30,8 @@ require (

replace github.com/pion/interceptor v0.1.12 => github.com/streamer45/interceptor v0.0.0-20230202152215-57f3ac9e7696

replace github.com/mattermost/mattermost-plugin-calls/server/public => ./server/public
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually tried to use Go workspaces to avoid this but ended up in a bit of rabbit hole with some unexpected failures on dependencies. Will look into this at another time as it's working fine for now.

@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2023 •
edited
Loading

Codecov Report

Attention: 78 lines in your changes are missing coverage. Please review.

Comparison is base (6c5f9a1) 5.34% compared to head (30920ae) 5.32%.
Report is 3 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff            @@
##            main    #542      +/-   ##
========================================
- Coverage   5.34%   5.32%   -0.03%     
========================================
  Files         24      24              
  Lines       4394    4469      +75     
========================================
+ Hits         235     238       +3     
- Misses      4141    4214      +73     
+ Partials      18      17       -1
Files Coverage Δ
server/session.go 0.00% <0.00%> (ø)
server/bot_api.go 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@streamer45 streamer45 modified the milestones: v0.21.0, v0.20.0 / MM 9.1 Oct 3, 2023
cpoile
cpoile reviewed Oct 3, 2023
View reviewed changes
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Just one question:

server/bot_api.go Outdated
Comment on lines 295 to 296
state.Call.Recording.EndAt = time.Now().UnixMilli()
state.Call.Recording.Err = status.Error
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm misreading, but every api hit will assume you're stopping the recording? We're not using the Status field?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are correct. We are only handling one status at the moment so it doesn't matter but yes I definitely need to add a check as we'll be soon using it for the started event as well. I'll send an update 👍

@streamer45 streamer45 requested a review from cpoile October 3, 2023 20:21
cpoile
cpoile approved these changes Oct 3, 2023
View reviewed changes
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Awesome, makes more sense now (even if only logically). Thanks!

@streamer45 streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Oct 3, 2023
@streamer45 streamer45 added 2: Dev Review Requires review by a core committer and removed 3: Reviews Complete All reviewers have approved the pull request labels Oct 4, 2023
@streamer45 streamer45 requested a review from cpoile October 4, 2023 18:21
@streamer45 streamer45 added 3: Reviews Complete All reviewers have approved the pull request Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) and removed 2: Dev Review Requires review by a core committer labels Oct 4, 2023
@streamer45 streamer45 removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Oct 5, 2023
@streamer45 streamer45 merged commit 3fffd87 into main Oct 5, 2023
@streamer45 streamer45 deleted the MM-54589 branch October 5, 2023 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpoile cpoile cpoile approved these changes

Assignees

@streamer45 streamer45

Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Milestone
v0.20.0 / MM 9.1 & MM 9.2
Development

Successfully merging this pull request may close these issues.
3 participants
@streamer45 @codecov-commenter @cpoile