feat(shell-api): add automerge information to sh.status() MONGOSH-1649 #2422
Conversation
| }; | ||
| })(), | ||
| (async (): Promise<void> => { | ||
| // Is automerge currently enabled, available since >= 7.0 |
There was a problem hiding this comment.
I'm using null scenario to return true to match the mongod behavior. But this is available since 7.0 which means in older versions it'd also similarly say 'yes' which is weird.
The solution to this is to check for the server version and compare with semver but I don't see this being used anywhere
- Do we want to introduce this kind of differentiation and hide this field < 7.0?
- If yes, what does a server version mean for automerge in sharded cluster? If I understand right it's possible for shards to have i.e. mix of 6.0 and 7.0 Mongo versions, does one get it from i.e. adminDB or something of sort?
There was a problem hiding this comment.
Just wondering – what does automerge === null correspond to? Is that the case in which we would want to not print this information?
There was a problem hiding this comment.
Going off https://github.com/mongodb/mongo/blob/a79ad6aa8b026d511227c4ece3d8c80265578831/src/mongo/shell/utils_sh.js#L264 my guess is that the idea here is that automerge wasn't explicitly enabled but it is enabled by default in 7.x+ so they print it out as such
There was a problem hiding this comment.
Yeah just confirmed this more or less with start/stop autoMerger is when that'd appear.
We could omit it when the information isn't explicitly set and show only if it has been configured; users looking for it would probably end up starting/stopping automerger so seems reasonable, though a bit of divergence from what we usually do
| }); | ||
| describe('automerge', function () { | ||
| it('not shown if sh.status() if not explicitly enabled', async function () { | ||
| expect((await sh.status()).value.automerge).is.undefined; |
There was a problem hiding this comment.
this works but does kind of mess with the sh.status integrity a little as we don't really hide fields beyond that.
I'm unsure how problematic would determining the implicit default status based on the version would be but that seems to be the only other option (or setting the status to 'unknown' but that is also a bit weird and would be shown to a lot of users by default). If we went that route, the version would probably be derived from the admin DB right?
There was a problem hiding this comment.
If we went that route, the version would probably be derived from the admin DB right?
The server version? The only reliable version that clients are supposed to make decisions on is the wire version reported by hello
There was a problem hiding this comment.
hm okay, if omitting the field is fine then we could just go with that.
Thinking about it more, it's also a weird precedent to assume that because it's a certain version then lack of a setting means it is some set default from our end
| }); | ||
| describe('automerge', function () { | ||
| it('not shown if sh.status() if not explicitly enabled', async function () { | ||
| expect((await sh.status()).value.automerge).is.undefined; |
There was a problem hiding this comment.
If we went that route, the version would probably be derived from the admin DB right?
The server version? The only reliable version that clients are supposed to make decisions on is the wire version reported by hello
It's possible the setting can exist in the cluster we're using so we shouldn't have a test assuming it'd be undefined (and thus not show) for versions >= 7
We expose a status for the automerge being enabled/disabled so we'd want it to be shown in
sh.status()This does that in form: