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-53432] Refactor to support multiple job types #44

Merged
merged 14 commits into from
Nov 21, 2023
Merged

Conversation

streamer45
Copy link
Contributor

Summary

PR slightly refactors the service to allow for different job types, namely post call transcriptions.

One important breaking change here is that we'll start using the more idiomatic (and generic) /data volume path from now on. This means we'll need to bump the minimum recorder image version to the latest once released.

Ticket Link

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

@streamer45 streamer45 added 2: Dev Review Requires review by a core committer Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) labels Oct 9, 2023
@streamer45 streamer45 requested a review from cpoile October 9, 2023 23:18
@streamer45 streamer45 self-assigned this Oct 9, 2023
@streamer45 streamer45 added this to the v0.5.0 milestone Oct 9, 2023
Makefile Outdated
@@ -41,8 +41,8 @@ DOCKER_REGISTRY_REPO ?= mattermost/${APP_NAME}-daily
DOCKER_USER ?= user
DOCKER_PASSWORD ?= password
## Docker Images
DOCKER_IMAGE_GO += "golang:${GO_VERSION}@sha256:dd9ad81920b63c7f9f18823d888d5fdcc7e7516086fd16654d07bc437f0e2427"
DOCKER_IMAGE_GOLINT += "golangci/golangci-lint:v1.52.2@sha256:5fa6a92ab28ca3421c88d2b6cd794c9759d05a999aceca73053d014aad41b9d3"
DOCKER_IMAGE_GO += "golang:${GO_VERSION}@sha256:b17c35044f4062d83c815434615997eed97697daae8745c6dd39dc3673b87efb"
Copy link
Member

Choose a reason for hiding this comment

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

I ran into an issue with this today -- if we use a sha, does that mean you're pinning to a particular arch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, ideally we'd be using docker buildx given the way we are building is actually deprecated ( https://mattermost.atlassian.net/browse/MM-54412). That should also help with multi arch builds but will probably need some non trivial amount of work.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... For now I just have to manually remove the sha's build on mac, it's a pain, but I guess sec wouldn't be happy if we removed them here, eh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They asked for pinning when possible, even though from a security perspective it kinda forces us to keep it up to date in case of security fixes upstream, so it's not that great. I guess build reproduction is a better reason for keeping these.

Manual patching is the quickest thing for now to unblock testing/development I suppose.

@cpoile cpoile self-requested a review October 10, 2023 23:40
@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 10, 2023
@streamer45 streamer45 removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Nov 21, 2023
@streamer45 streamer45 merged commit be7044b into master Nov 21, 2023
3 checks passed
@streamer45 streamer45 deleted the MM-53432 branch November 21, 2023 21:47
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.

3 participants