GPII-3119 SPI Settings Refactor for Lookup Paths #832
Conversation
… it from capabilitiesTransforms until the acceptanceTests are reworked for it.
…re supportedSettings in test specs.
sgithens
changed the title
|
CI job failed: https://ci.gpii.net/job/universal-tests/2047/ |
|
@sgithens, thanks for solving this long pending issue. Please fix the failing browser tests. |
|
CI job passed: https://ci.gpii.net/job/universal-tests/2048/ |
|
This pull request looks good to me. @amb26, @stegru, what do you think? Its accompanying windows repo pull request is GPII/windows#289. |
|
I don't understand why the path went into supportedSettings rather than options. It is not actually a supported setting at all. Also, the stray "undefined" mouse droppings in the test cases should be cleaned up. |
|
The For the
|
|
OK, sorry I misread the nesting structure of the "supportedSettings" block. The "undefined" business still seems cabberwonky though, there are indeed places in the LM where we are sensitive to undefined vs no key, but this shouldn't be one of them. We should either i) fix up the site in the LM where we do the assignment so that it doesn't create the mouse dropping (preferred), or ii) fix up the tests so that they use some mechanism such as jqUnit.assertLeftHand or jqUnit.assertCanoniseEqual so that they are insensitive to the dropping. |
|
Cool, I will take a look at the usage of that method in the unit tests. Also, I hope this thread is indexed by search indexes for usage of the term |
|
CI job passed: https://ci.gpii.net/job/universal-tests/2071/ |
|
@amb26 Ok, I cleaned up the |
|
CI job passed: https://ci.gpii.net/job/universal-tests/2073/ |
Fixed merge conflict in gpii/node_modules/transformer/test/TransformerTests.js
|
CI job passed: https://ci.gpii.net/job/universal-tests/2090/ |
|
Updated and fixed merge conflict |
|
CI job passed: https://ci.gpii.net/job/universal-tests/2106/ |
|
@amb26 @cindyli Do we think this is ready to be merged now, along with it's sister PR in gpii-windows? GPII/windows#289 |
|
Hi, I merged this by accident, until I thought properly - we can't commit this yet because it makes a backwards-incompatible change to the structure of the solutions registry. Before we can do this, we need a robust solution to what we do when an old client requests SR entries from the cloud and ends up with this one, which implies that we need to add a version field into the SR, as well as one into each client matchmaking request, and implement a facility to translate modern SR entries back into arbitrarily ancient ones using some kind of "transforming chain". Without this, such an update will BREAK THE WORLD ... |
|
I've filed the blocking issue as https://issues.gpii.net/browse/GPII-4329 - please reopen this PR |
| "properties": { | ||
| "schema": { "$ref": "gss-v7-full#", "required": true } | ||
| "schema": { "$ref": "gss-v7-full#", "required": true }, | ||
| "path": { |
There was a problem hiding this comment.
This seems like an inappropriate place to put this. As it stands you're basically saying that any setting might have a path, and that it has to be either a string or object. The individual settings should define the fact that they use the path.
I'd also argue that you could be a lot more specific than just accepting an object. I assume we know what properties such an object would have, i. e. we can only accept get and set, and it must have both of them to be a usable object. I realise that this results in a lot of repeated boilerplate, but this will go away with the LSR work, where we can have a grade for all SPI settings where we express this once.
| } | ||
| } | ||
| }, | ||
| "path": "pvParam" |
There was a problem hiding this comment.
We should maybe discuss this in our next meeting, I think with a little work this could be a very positive change. Right now, the schema still allows path, but you'd in essence ignore that and clobber it with whatever was defined as an "option" outside of the schema. That seems a bit dishonest, we should not advertise path as anything the user can control if we're gonna ignore them.
I'd argue that if you're gonna do this, you should go ahead collapse the value property as well, so that the transforms can either be removed or just become a simple map of (legacy) user-facing settings to settings-handler-facing settings.
5 participants
Removal of invariant path information from a settings capabilities transforms. Removes the need for cludgy windows workarounds when snapshotting and reading settings. Also makes more sense.
This PR should be considered a prerequisite for GPII-228 (snapshotting) as it fixes the crashes from missing SPI settings data.