Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GPII-4273iii LFM gets repository revision and computes URL to solutions registry therein #840

Open
wants to merge 93 commits into
base: master
Choose a base branch
from

Conversation

klown
Copy link
Member

@klown klown commented Jan 24, 2020

Implementing part (iii) of GPII-4273 where the LFM calls the /revision endpoint of the CBFM and uses the result to compose the full URL to the solutions registry in universal's github repository.

@cindyli @amb26 I'm doing this pull to see how the changes fare against CI, and I may change things further. Certainly, something needs to be done in terms of specifying the actual file name win32.json5, currently hard-coded in the gpii.flowManager.config.local.base.json5 configuration file. It should come from somewhere in the windows implementation. That said, feel free to look it over and make comments if you like.

NOTE: This includes code changes from PR #837 (part ii of GPII-4273) and PR #836 (part i).

JIRA: https://issues.gpii.net/browse/GPII-4273

@gpii-bot
Copy link

CI job failed: https://ci.gpii.net/job/universal-tests/2093/

Includes changes from master, GPII-4306, and GPII-4273ii.
@gpii-bot
Copy link

CI job failed: https://ci.gpii.net/job/universal-tests/2105/

Also, rationalized capitalization of "URL"
@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2113/

@klown
Copy link
Member Author

klown commented Jan 28, 2020

@cindyli @amb26 Ready for review, although I'm still looking into how to parameterize the name of the solutions registry file, or if that is even necessary.

@amb26
Copy link
Member

amb26 commented Jan 29, 2020

Thanks, @klown, this work is very nearly ready. The solution to your parameterisation problem - the implementation should not hard-code the required filename since this will be platform-dependent. Instead, this fetch should be implemented as an identical workalike to the already existing solutionsRegistryDataSource at https://github.com/GPII/universal/blob/master/gpii/node_modules/flowManager/src/FlowManager.js#L45 and https://github.com/GPII/universal/blob/master/gpii/node_modules/flowManager/src/SolutionsRegistryDataSource.js#L73 - the client supplies the output of the platformReporter as the directModel if necessary, and the impl sorts out which file to dispense.
It would be worth trying to trace out how that.options.path in the solutionsRegistryDataSource gets set correctly since right now all I can see is https://github.com/GPII/universal/blob/master/gpii/node_modules/flowManager/configs/gpii.flowManager.config.base.json5#L13 which sets it just to the base directory. How does it figure out from a site like https://github.com/GPII/universal/blob/master/gpii/node_modules/flowManager/src/MatchMaking.js#L96 what is the required filename? @cindyli may be able to help with detective work

@klown
Copy link
Member Author

klown commented Jan 29, 2020

Thanks @amb26. Re: the detective work: I had started, and noticed that the SolutionsRegistryDataSource currently pulls in all of the platform solutions in the solutions directory, but something else chooses the actual file. That's what I meant by whether it was even necessary for this to provide the file name when something else is doing that.

I'm also reminded of something I did in 2017, making the PlatformReporter context aware, and having it provide the OS info. I'm going to see if that has any use here (windows specific implementation). It might just be the solution to the problem involves the current platform/device reporter components. We'll see.

@klown
Copy link
Member Author

klown commented Jan 29, 2020

@amb26 @cindyli Looks like it might be as simple as: LFM.deviceReporter.platformReporter.reportPlatform()

On macOS, that gives: { id: 'darwin', version: '18.7.0' }, where the id is the actual filename, without the .json5 extension. I'll confirm that the id is win32 when run on Windows.

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2115/

@amb26
Copy link
Member

amb26 commented Jan 30, 2020

Thanks @klown - but how does it actually pull in all the platform solutions? All I found was a single require statement...

@klown
Copy link
Member Author

klown commented Jan 30, 2020

@amb26, Someone very clever put an index.js in the solutions registry folder, which contains a require() statement for each solutions file in the folder. The algorithm for require() has a rule that if there is an index.js in the folder required, load it.

I wonder if there is some way to use node's package manager to get the solutions out of the repository -- treat the solutions registry as a module.

@amb26
Copy link
Member

amb26 commented Jan 30, 2020

@klown - well spotted! I feel sure that such a piece of deviousness could only possibly have been the evil work of KASPARNET @kaspermarkus

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2122/

@gpii-bot
Copy link

gpii-bot commented Feb 5, 2020

CI job passed: https://ci.gpii.net/job/universal-tests/2127/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2289/

@gpii-bot
Copy link

CI job failed: https://ci.gpii.net/job/universal-tests/2290/

@klown
Copy link
Member Author

klown commented Apr 27, 2020

ok to test

@gpii-bot
Copy link

CI job failed: https://ci.gpii.net/job/universal-tests/2291/

@klown
Copy link
Member Author

klown commented Apr 28, 2020

ok to test

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2292/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2301/

@gpii-bot
Copy link

gpii-bot commented May 5, 2020

CI job passed: https://ci.gpii.net/job/universal-tests/2310/

@gpii-bot
Copy link

gpii-bot commented May 7, 2020

CI job passed: https://ci.gpii.net/job/universal-tests/2316/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2319/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2325/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2333/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2347/

@gpii-bot
Copy link

gpii-bot commented Jun 8, 2020

CI job passed: https://ci.gpii.net/job/universal-tests/2352/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2374/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2385/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2388/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants