Repository navigation
Read a list setting given as one comma separated string, as configuration files give it - #129
Merged
Merged
Conversation
…uration files give it
…tion files give it, in the engines and usage sharing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A list setting can be given only as an array, though a configuration file gives a list as one comma separated string. So a
51d.jsonthat gives a list the way the specification says fails or is misread in three places, atrc6d5d4fe.restrictedPropertiesof an on-premise engine. Device Detection and IP Intelligence callrestrictedProperties.forEach(deviceDetectionOnPremise.jslines 376 and 377,ipIntelligenceOnPremise.jslines 256 and 257), and the baseEnginecallsrestrictedProperties.map(engine.jslines 65 to 75), so a string stops the build with aTypeError.headerBlacklistof usage sharing. The share filter callsmapon it (shareUsageEvidenceKeyFilter.jsline 80), so a string stops the build too.queryWhitelistof usage sharing. Usage sharing spreads it into an array (shareUsage.jslines 174 to 176), so"a,b"becomes the parameter namesa,,andb, with no error.What the specification says
From
pipeline-specification/features/pipeline-configuration.mdlines 50 to 61 at 706de1f0:What changed
listSettinginfiftyone.pipeline.core, exported from the package, reads a list setting. An array is used as it is. A string is split at each comma, each item without the space around it and with empty items left out. A string with no items counts as not set, because an empty list would restrict an engine to no properties.Engine, both on-premise engines (restrictedProperties) and usage sharing (queryWhitelistandheaderBlacklist) read their lists through it.listSetting, written in the formnpm run tscwrites.An array given in code works as before, so nothing changes for a caller that already gives one.
Tests
pipeline/fiftyone.pipeline.core/tests/listSetting.test.jschecks the six cases oflistSetting, being a comma separated string, a string with no comma, empty items and spaces, a string with no items, an array and a setting that is not set.pipeline/fiftyone.pipeline.engines/tests/restrictedPropertiesText.test.jschecks that an engine givenrestrictedPropertiesas a string restricts itself to those properties, and as an array to that array.restrictedPropertiesText.test.jsin each on-premise package builds the engine from a configuration whoserestrictedPropertiesis one comma separated string, on the free data file, and checks the properties it offers.pipeline/fiftyone.pipeline.engines.fiftyone/tests/shareUsageConfiguration.test.jsbuilds usage sharing from a configuration with both lists as comma separated strings, then as arrays, and checks which query parameters and headers reach the message.Red, run 43639258221 on 41c6e40, the test commit on
rc's code, with only a throwaway workflow added that builds the native modules and fetches the free data files as CI does. On Ubuntu 10 of the 12 tests fail, being the sixlistSettingtests, asrchas nolistSetting, and the four that give a list as a string. The two that give a list as an array pass.Green, run 43639269745 on d17526e, the fix, with only the same throwaway workflow added. All 12 pass, and lint passes on the four packages the change touches with the three warnings
rcalready has.Types, run 43639283318 ran
npm run tscon d17526e merged with the head of #113, which writes the types again. The declarations it wrote for these settings and forlistSettingare committed here as dec76a2. Its other difference,pipeline/fiftyone.pipeline.did/types/owid-js/v1.d.ts, comes from the OWID submodule already onrc, which #127 decides.Full build
Run 43650210325 on
tmp/pr-matrix-129, started bytools/pr-matrix.pywithout macOS. All 7Setting 'PullRequestSha'lines give this head, dec76a2, and Configure printed "Left out 2 macOS entries". Each of the six Build and Test jobs, Ubuntu, Ubuntu ARM and Windows on Node.js 22 and 24, passed 1,505 unit tests with 2 skipped and 96 integration tests with 4 skipped.macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.
What remains
Nothing for this requirement. The other list setting of the packages, the cloud engines'
requestedProperties, already takes one comma separated string. The pipeline builders passrestrictedPropertiesto the engines as given (for exampledeviceDetectionOnPremisePipelineBuilder.jsline 174), so a string given to them is read the same way.