Plugins: update how investUsageLogger gets port information #1691
Conversation
| @@ -112,6 +114,7 @@ export function setupInvestRunHandlers() { | |||
| 'run', | |||
| modelRunName, | |||
| `-d "${datastackPath}"`]; | |||
| port = settingsStore.get('core.port'); | |||
There was a problem hiding this comment.
I think this solution is fine. I was wondering if we could always just use the core invest server for usage logging, even when running a plugin, but I see that natcap.invest.usage._log_model needs to import the model.
| @@ -225,7 +225,8 @@ describe('createWindow', () => { | |||
| }); | |||
|
|
|||
| describe('investUsageLogger', () => { | |||
| const expectedURL = `http://127.0.0.1:${process.env.PORT}/api/log_model_start`; | |||
There was a problem hiding this comment.
I see that process.env.PORT is still being defined when we run yarn run test (see package.json). Maybe we can and should remove that from the yarn scripts if we're no longer using it.
workbench/package.json
Outdated
| @@ -8,10 +8,10 @@ | |||
| "start": "yarn build-main && yarn build:preload && concurrently --kill-others \"yarn serve\" \"electron .\"", | |||
| "serve": "cross-env MODE=development node scripts/watch.js", | |||
| "lint": "eslint --cache --color --ext .jsx,.js src", | |||
| "test": "cross-env PORT=56788 jest --runInBand --testPathIgnorePatterns /tests/binary_tests/ /tests/sampledata_linkcheck/", | |||
| "test": "cross-env jest --runInBand --testPathIgnorePatterns /tests/binary_tests/ /tests/sampledata_linkcheck/", | |||
There was a problem hiding this comment.
cross-env is the program for setting environment variables in a cross-platform way, so we don't need that anymore.
There was a problem hiding this comment.
Good catch, I didn't realize they were related. updated in 5cc7ffe
There was a problem hiding this comment.
Nothing to add to Dave's comments, I think this is a good solution. It could be a little cleaner to look up the port and pyModuleName in the settings store using the modelRunName - if modelRunName is in settingsStore.get('plugins'), then it's a plugin. There's an example of this on line 93 of setupInvestHandlers.js in the diff.
Thanks @emlys , I was looking into this for a solution because I thought it'd be cleaner too. But, for the let port;
function start() {
port = "logic to get port from pyModuleName";
}
function end(){
"use port, which should be initialized by the time this call occurs";
} |
|
Thanks Dave, I see some tests failing related to GDAL 3.10, which we fixed in |
Description
Since we're now managing multiple invest servers on different ports, we're storing related port information in the
settingsStoreconfig.investUsageLoggerwas still usingprocess.env.PORT. This PR attempts to transition that to the new setup.I'd love some feedback and input on how to manage this. The solution I have here works, but feels a little tacked on. I couldn't think of a clean way for
investUsageLoggerto locally know if a plugin or core model was being run to get the port information there. So, I end up passing the port in to thestartandexitfunctions fromsetupInvestHandlers.