Why would a development thing be enabled on a production server? What is a coredev endpoint?

In T247997#5981939, @Krenair wrote:

Why would a development thing be enabled on a production server? What is a coredev endpoint?

See T232485, search that page for "v0". That link does not specifically mention "coredev". Per later discussion (which I couldn't quickly find a link for) "experimental" endpoints implemented in core are allowed to be located under coredev/v0.

As for why a development thing would be enabled on a production server, one reason is that many of these endpoints are created to serve the iOS and Android apps, and some final coordination/confirmation between those teams and Core Platform Team is necessary before an endpoint is marked "official" and therefore available for general use by any caller. We do, of course, coordinate with those teams before and throughout development, but sometimes when you see the finished, working product you realize it needs a tweak. We don't want to release a new endpoint, have the mobile devs tell us it wasn't quite what they needed, and then immediately have to go through a deprecation process.

Anything pushed to master, even the endpoints under coredev/v0, has gone through the full, normal code review process, the same as any other production code. Don't think of "development thing" in terms of "unfinished/buggy" work. Think of it in terms of "ready for final signoff".

I hope that helps.

ah, so it's a simple feature flag for use while the feature is under active development, and the API is not stable yet. and the flag might not even last to the next MW release? thanks Bill

We would like to default the configuration variable to false, so that endpoints not covered by the deprecation policy do not suddenly become available (even under the /coredev/v0 path) on wikis that have not explicitly chosen to enable them. We don't want to do anything surprising for third party installs, etc.

However, the endpoints will need to continue to function normally when tests are run during continuous integration (thanks, @Clarakosi, for raising this concern).

How do we set/inject the proper "true" value into continuous integration? @hashar, your name was mentioned when CPT discussed this today. Can you give guidance here?

@Krenair raised a good point. Is $wgEnableDevelopmentEndpoints the name we prefer? The RFC used the terms "unstable" (shorter, but a little scary) and "experimental" (doesn't really inspire much confidence either).

On reflection, I'm leaning toward $wgEnableUnstableEndpoints, where "stable" is thought of in the same sense as the Stable Interface Policy. "Unstable" just means that it might change or go away, not that it is buggy or incomplete. It is also a bit shorter.

However, I don't have strong feelings, and if everyone else is happy with the originally proposed name, I'll implement it per the task description.

I mainly just wanted to check what the nature of this was and that it wasn't something akin to wgEnableAPI that would need to be maintained long term. My intention was not to start a bikeshed :)

In T247997#5982631, @Krenair wrote:

I mainly just wanted to check what the nature of this was and that it wasn't something akin to wgEnableAPI that would need to be maintained long term. My intention was not to start a bikeshed :)

Fair enough, I'll take the blame for any bikeshedding. :) The immediate intention of this flag is to ensure that system administrators have all tools necessary to keep the sites up in the face of surprises or attacks, even if the pandemic results in reduced staff availability. Basically, we want to give them a simple kill switch covering any of our current work that isn't necessary for the site to function. So I promise I won't let this sit around for long waiting on a discussion about names.

CI sources mediawiki/core file includes/DevelopmentSettings.php. So I guess you could enable the experimental / development endpoint there.

It would defaults to false via includes/DefaultSettings.php and for Wikimedia wiki it can be turned on a per language/project/wiki base via mediawiki-config.

Change 581885 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/core@master] Add feature flag $wgEnableRestAPIDevelopmentEndpoints

https://gerrit.wikimedia.org/r/581885

Change 581886 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[operations/mediawiki-config@master] Add configuration variable $wgEnableRestAPIDevelopmentEndpoints

https://gerrit.wikimedia.org/r/581886

Change 581887 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/extensions/OAuth@master] Adjust EndpointTest for new Router constructor signature

https://gerrit.wikimedia.org/r/581887

Change 581887 abandoned by BPirkle:
Adjust EndpointTest for new Router constructor signature

Reason:
Not needed after all.

https://gerrit.wikimedia.org/r/581887

@daniel had the excellent suggestion that we implement this by placing the development/experimental routes into a separate .json file, and include that based on a config variable. So instead of a boolean feature flag to enable/disable development endpoints, we would add something like $wgRestAPIAdditionalRouteFiles, which would be an array of additional route files to include. Then in includes/DevelopmentSettings.php, we would set that to include a new file, something like coreDevelopmentRoutes.json.

This accomplishes the same objective (giving system admins an easy way to turn off development routes), it greatly simplifies the implementation, and it also has potential for general use beyond just controlling availability of development routes.

@eprodromou , any objections?

If we want the config to still be a simple flag, EntryPoint could just decide based on this flag which files to pass on to the Router instance.

In T247997#5988036, @daniel wrote:

If we want the config to still be a simple flag, EntryPoint could just decide based on this flag which files to pass on to the Router instance.

True.

I feel like an array of route files is sufficiently clear and easy to disable for system admins, while also being more flexible than a flag. Also, I currently have it coded in a very conservative way, with the coreDevelopmentRoutes.json file only included on testwiki. (Well, and for CI via DevelopmentSettings.php, but testwiki is the only actual wiki the development routes would be avaialble on.) So there's very little chance the development routes will be a problem - it is more likely that we'll want to try them out against a wiki they're not available on, but I thought right now it was better to err on the side of safety.

I don't have strong feelings about flag vs. array of route files, so if you or others see a compelling reason to do a flag instead, I'm happy to change it. I'm also happy to add a few more wikis in addition to testwiki if there are others that anyone thinks would be useful,.

I'm almost done coding the new approach, so in a few minutes I'll post what I have now. We can adjust as needed.

Change 581885 merged by jenkins-bot:
[mediawiki/core@master] Add config variable $wgRestAPIAdditionalRouteFiles

https://gerrit.wikimedia.org/r/581885

Change 581886 merged by jenkins-bot:
[operations/mediawiki-config@master] Add configuration variable $wgRestAPIAdditionalRouteFiles

https://gerrit.wikimedia.org/r/581886

Mentioned in SAL (#wikimedia-operations) [2020-03-23T18:14:27Z] <urbanecm@deploy1001> Synchronized wmf-config/InitialiseSettings.php: SWAT: 5eb70ac: Add configuration variable $wgRestAPIAdditionalRouteFiles (T247997) (duration: 01m 00s)

Mentioned in SAL (#wikimedia-operations) [2020-03-23T18:15:52Z] <urbanecm@deploy1001> Synchronized wmf-config/InitialiseSettings.php: SWAT: 5eb70ac: Add configuration variable $wgRestAPIAdditionalRouteFiles (T247997; take II) (duration: 00m 59s)

@BPirkle great implementation, thanks for doing it.

@eprodromou did you sign it off? Should it move to documentation?

Change 906554 had a related patch set uploaded (by Muehlenhoff; author: Muehlenhoff):

[operations/puppet@production] smart: Disable smart-dump for servers with hpsa

https://gerrit.wikimedia.org/r/906554