-
Notifications
You must be signed in to change notification settings - Fork 42
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
base: master
Are you sure you want to change the base?
Conversation
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.
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. |
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.
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?
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.
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
orServerURL
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.
Summary
Added a new
ServerScheme
configuration paramater for the deployer to allow usinghttps
on custom deployments, automatically setting upwss
if that is used.Ticket Link
https://mattermost.atlassian.net/browse/MM-61915