Add getWikiHostsForDomain endpoint#1001
Conversation
outdooracorn
left a comment
outdooracorn
left a comment
There was a problem hiding this comment.
Noticed something we should probably address, but let's do that as a follow-up PR.
tarrow
left a comment
tarrow
left a comment
There was a problem hiding this comment.
only the comment removal is blocking. The others are suggestions
outdooracorn
changed the title
outdooracorn
left a comment
outdooracorn
left a comment
There was a problem hiding this comment.
Made some nitpick improvements. I'm happy with this now. :)
|
|
||
| public function getBackendHostForDomain(string $domain): string { | ||
| // TODO: Move 'backend.default.svc.cluster.local' to an env variable (e.g. PLATFORM_MW_BACKEND_HOST_SUFFIX) for flexibility. | ||
| // TODO: Make host format configurable for flexibility |
There was a problem hiding this comment.
I actually found the previous comment clearer but I am also fine with updating it as this now
There was a problem hiding this comment.
The reason I changed it is because I don't actually know if PLATFORM_MW_BACKEND_HOST_SUFFIX is the best way to make this configurable. 😅 What about the mediawiki- at the start? Should it be PLATFORM_MW_BACKEND_HOST_FORMAT=mediawiki-%s-app-backend.default.svc.cluster.local. That won't work for the other hosts (web, api, alpha) though. Do we have one env var per host? Or one env var for all hosts, e.g. PLATFORM_MW_HOSTS_FORMAT=mediawiki-{version}-app-{name}.default.svc.cluster.local with strtr?
dati18
left a comment
dati18
left a comment
There was a problem hiding this comment.
I tested this locally. I also need it for other work so let's move it :D
tarrow
dismissed
their stale review
thanks for pointing out my mistake about the comment; let's follow up
5 participants
Bug: T409530
Co-author: @outdooracorn