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-61915] feat: support configurable scheme for serverURL #879

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fmartingr
Copy link
Contributor

Summary

Added a new ServerScheme configuration paramater for the deployer to allow using https on custom deployments, automatically setting up wss if that is used.

Ticket Link

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

@fmartingr fmartingr added the 2: Dev Review Requires review by a core committer label Jan 13, 2025
@fmartingr fmartingr self-assigned this Jan 13, 2025
Copy link
Member

@agarciamontoro agarciamontoro left a comment

Choose a reason for hiding this comment

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

Oh, neat, thank you! A nice addition here would be to centralize building the scheme in a util function so that we can refactor out all of those t.config.ServerScheme + "://". Thoughts?

@@ -119,6 +119,8 @@ type Config struct {
// Mattermost servers. This is used to override the server URL in the agent's config in case there's a
// proxy in front of the Mattermost server.
ServerURL string `default:""`
// ServerScheme is the scheme to use when connecting to the Mattermost server.
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I the only one who thinks this is getting slightly confusing?

We already have two settings to override URLs in slightly different ways, and now we are adding a third to control the scheme portion. I think that eventually we'll need to take a step back and try to simplify some of it because we are starting to tweak things for any little new use case we find. @agarciamontoro Thoughts?

Also, what happens if either SiteURL or ServerURL already have a scheme set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Am I the only one who thinks this is getting slightly confusing?

No 😬 It already was, honestly, based on the logic around the code regarding the URLs, the proxy, etc.

We already have two settings to override URLs in slightly different ways, and now we are adding a third to control the scheme portion. I think that eventually we'll need to take a step back and try to simplify some of it because we are starting to tweak things for any little new use case we find. @agarciamontoro Thoughts?

The problem here is what to use in what places. Honestly I would just use a complete setting (a complete URL) as the URL the clients should connect to, that being a node, a proxy, a load balancer... and leaving it empty would try to guess what to use from the configuration.

Also, what happens if either SiteURL or ServerURL already have a scheme set?

it won't work since the code prepends the scheme (both before and after this change)

I will let you discuss this a bit internally before making more changes to the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants