-
Notifications
You must be signed in to change notification settings - Fork 20
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
Citrine demo that is broken but downloads and runs all docker contain… #45
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@louisg1337 please edit the existing demo script by specifying additional command line options instead of creating a new script. Also, please remember to sign your commits with -s, everest enforces DCO https://github.com/apps/dco
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I try to run this without changes, I get
npm verb cli [
npm verb cli '~/.nvm/versions/node/v13.12.0/bin/node',
npm verb cli '~/.nvm/versions/node/v13.12.0/bin/npm',
npm verb cli 'run',
npm verb cli 'compile',
npm verb cli '--workspaces',
npm verb cli '--verbose'
npm verb cli ]
npm info using [email protected]
npm info using [email protected]
npm verb stack Error: missing script: compile
npm verb stack at run (~/.nvm/versions/node/v13.12.0/lib/node_modules/npm/lib/run-script.js:155:19)
npm verb stack at ~/.nvm/versions/node/v13.12.0/lib/node_modules/npm/lib/run-script.js:63:5
npm verb stack at ~/.nvm/versions/node/v13.12.0/lib/node_modules/npm/node_modules/read-package-json/read-json.js:116:5
npm verb stack at ~/.nvm/versions/node/v13.12.0/lib/node_modules/npm/node_modules/read-package-json/read-json.js:436:5
npm verb stack at checkBinReferences_ (~/.nvm/versions/node/v13.12.0/lib/node_modules/npm/node_modules/read-package-json/read-json.js:391:45)
npm verb stack at final (~/.nvm/versions/node/v13.12.0/lib/node_modules/npm/node_modules/read-package-json/read-json.js:434:3)
npm verb stack at then (~/.nvm/versions/node/v13.12.0/lib/node_modules/npm/node_modules/read-package-json/read-json.js:161:5)
npm verb stack at ~/.nvm/versions/node/v13.12.0/lib/node_modules/npm/node_modules/read-package-json/read-json.js:382:12
npm verb stack at ~/versions/node/v13.12.0/lib/node_modules/npm/node_modules/graceful-fs/graceful-fs.js:115:16
npm verb stack at FSReqCallback.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:63:3)
npm verb cwd /private/var/folders/y5/cx3cfzrd2q116myv9ly86sw1rnlmdj/T/tmp.TqHBaXyy/citrineos-csms
npm verb Darwin 22.6.0
npm verb argv "/Users/kshankar/Desktop/data/.nvm/versions/node/v13.12.0/bin/node" "/Users/kshankar/Desktop/data/.nvm/versions/node/v13.12.0/bin/npm" "run" "compile" "--workspaces" "--verbose"
npm verb node v13.12.0
npm verb npm v6.14.4
npm ERR! missing script: compile
npm verb exit [ 1, true ]
npm timing npm Completed in 116ms
npm ERR! A complete log of this run can be found in:
npm ERR! ~/.npm/_logs/2024-05-27T16_53_48_231Z-debug.log
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! @citrineos/[email protected] build: `npm run compile --workspaces --verbose`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the @citrineos/[email protected] build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm ERR! A complete log of this run can be found in:
npm ERR! ~/.npm/_logs/2024-05-27T16_53_48_263Z-debug.log
Error: Failed to build the project.
|
||
if [[ -f "$CITRINE_DOCKER" ]]; then | ||
# Use sed to find and replace the string | ||
sed -i '' 's/8082:8082/80:80/g' "$CITRINE_DOCKER" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@louisg1337 does this actually work? Are you able to connect to localhost:80
after this? I bet not. I would assume that you need 8082:80
instead. Please check the docker-compose docs!
The failure is because Citrine needs node 18, and I have node 13 installed by default. But for the one-line demo, we can't assume that the users have node 18 installed. We should also not upgrade their default version of node without letting them know. We should use a docker container for the citrine build as well, not just for the citrine dependencies. |
@louisg1337 I switched this to port 8081 and am able to connect the station to the CSMS
However, when I try to authenticate, the token is not registered. @louisg1337 can you see how to configure the token?
|
I interpret the instructions here #44 (comment) as using the "Monitoring" section of the API.
I used the Swagger API to make a GET for
And got this response
There is no |
I think we can pause this for now since Citrine does not support Security Profile 3 anyway. But I think that if/when we get back to this, it will work. |
For the record, I figured out an option to add tokens (this is using the DirectUS admin UI at http://localhost:8055 |
Alas, I can't figure out where to put in the token string through the UI. But we are even closer! |
I am not able to edit this branch directly, so I have added the 3 line patch here for the record. Testing done: #45 (comment) |
Hi @shankari and @louisg1337 , for some local testing on my end I had forked this repo and run it with citrine. To add a location and password I created a small init script. See here: https://github.com/ChrisWeissmann/everest-demo/blob/main/citrineos/init.sh However since I was not working on SP3 things, I did not put in the effort to add support for it in the single line script. |
demo-iso15118-2-ac-plus-ocpp.sh
Outdated
MAEVE_REPO="https://github.com/thoughtworks/maeve-csms.git" | ||
MAEVE_BRANCH="b990d0eddf2bf80be8d9524a7b08029fbb305c7d" # patch files are based on this commit | ||
CSMS_REPO="https://github.com/thoughtworks/maeve-csms.git" | ||
# MAEVE_BRANCH="b990d0eddf2bf80be8d9524a7b08029fbb305c7d" # patch files are based on this commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not remove this. The patches won't won't at an arbitrary point in the repo.
We should have a separate branch for the CITRINE repo
demo-iso15118-2-ac-plus-ocpp.sh
Outdated
exit 1 | ||
fi | ||
else | ||
git reset --hard ${MAEVE_BRANCH} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to restore the line above so that this can run
demo-iso15118-2-ac-plus-ocpp.sh
Outdated
exit 1 | ||
fi | ||
|
||
docker-compose -f ./docker-compose.yml up -d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker-compose.yml
is the default file, so no need to specify this here
# Set up CSMS | ||
echo "Setting up ${CSMS}" | ||
if [[ "$CSMS" == "citrine" ]]; then | ||
npm run install-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I indicated earlier, this will not work for all versions of npm.
You need to install nvm or run this within a docker container.
#45 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure, I was working on figuring that out next, just wanted to get the initial working version out first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is not the first priority. We can document this for now and focus on fixing the other issues:
- specifying a specific branch
- scripts for basic auth and token
first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please incorporate the scripts from #45 (comment)
@louisg1337 you also need to address the DCO |
…ers successfully Signed-off-by: louisg1337 <[email protected]>
…the script using Citrine, only supports SP1 right now though. Signed-off-by: louisg1337 <[email protected]>
… HTTP auth now works Signed-off-by: louisg1337 <[email protected]>
Signed-off-by: louisg1337 <[email protected]>
I just pushed out a working version of the one line demo that will run Citrine and connect to EVerest using SP1 (thanks to @ChrisWeissmann). There are a few things to note.
Testing Done
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@louisg1337 I am fine with merging as soon as the citrine-demo-...
file is deleted, and you have edited the README to highlight how to test Citrine versus MaEVe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@louisg1337 please delete this file since the changes have been incorporated into the same demo-xxx
file. This will avoid confusion.
local response=$(curl -s -X POST "$login_url" -H "Content-Type: application/json" -d "$json_body") | ||
|
||
# Extract token from the response | ||
local token=$(jq -r '.data.access_token' <<< "$response") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not going to hold up the review for this, but, similar to my previous comment about npm
, our goal is to not request additional software installation. The goal behind the "single line demo" is that you have docker installed, and then run one command, and we launch the demo without making permanent changes to your laptop.
As the complexity of these scripts increases, in a future fix, I would suggest pulling them out into their own directory and launching them from the manager, or a docker container of their own that has all the required dependencies.
demo-iso15118-2-ac-plus-ocpp.sh
Outdated
if [[ ${CSMS} == "citrine" ]]; then | ||
echo "TODO: Copy over certs for Citrine!" | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a TODO to copy over certs, but a TODO to set up the device model correctly.
In the future fix, when we move this out to a script, instead of having the separate sp1.db
, sp2.db
, ...
we can just use SQLite to edit the device model.
Note also that the device DB should not depend on the CSMS - the whole point of having a standard is that, from a station perspective, it doesn't matter which CSMS it connects to. The station configuration (which is what this is) should be the same.
Signed-off-by: louisg1337 <[email protected]>
Signed-off-by: louisg1337 <[email protected]>
Sounds good to all the above. I just addressed the changes requested, so we should be all good. |
@louisg1337 there is an indentation issue caught by the static code analysis |
Signed-off-by: louisg1337 <[email protected]>
Signed-off-by: louisg1337 <[email protected]>
Signed-off-by: louisg1337 <[email protected]>
Co-authored-by: K. Shankari <[email protected]> Signed-off-by: Louis Grassi <[email protected]>
I don't see this getting fixed. I think it is a false positive. I am just going to merge this and we can fix (if needed) in the next PR. I don't want to waste our valuable time futzing around with this. |
Squash merging to have a clean commit history; @louisg1337 please pull appropriately. |
docker cp manager/device_model_storage_maeve_sp3.db \ | ||
everest-ac-demo-manager-1:/workspace/dist/share/everest/modules/OCPP201/device_model_storage.db | ||
if [[ ${CSMS} == "citrine" ]]; then | ||
echo "TODO: Set up device model correctly!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@louisg1337 I don't think we have to do any big device model changes. The main thing that we need to adjust is setting the CSMS url to also point to citrine. So we could just add an additional slot to the device model and adjust the priority. Then Everest would try both urls, based on priority, and one of them would work. Here is an example on what to adjust it to: https://github.com/ChrisWeissmann/everest-demo/blob/fe745cfdb0cb3c1abe48ab74e505454cf3d0c81b/manager/config.json
Setting this to the device model sqlite files that are in the repo, there won't need to really be a differentiation, if you are ok with EVerst trying both options. Or adding to the script a sqlite command that manipulates the network priority based on the CSMS.
…ers successfully