Adds Digital Twin Runner utility #110

prasadtalasila · 2023-09-15T14:24:51Z

This PR adds new Digital Twin Runner component to the project. This component can be run as a microservice. It can also be installed as a system utility that can be run as a regular command.

Current Status:

✔️ Working Nestjs project with useful functionality
✔️ Can be published as an npm package
✔️ Runs HTTP service and services two routes: /phase and /lifecycle/phase
✔️ Includes sufficient functionality for Digital Twin Lifecycle management. But not all of this functionality is available via HTTP API.
✔️ Complete integration and end-to-end tests
✔️ OpenAPI compliant-spec for DT Runner API (this yet to be implemented)

❌ Does not use dependency injection of nestjs framework
❌ Can not receive the commands over HTTP API
✖️ No unit tests
✖️ No configuration

❔ What is a good way to save the logs?
❔ Where should the configuration be saved? Can it be placed at a default location which is Operating System platform independent?

- Adds Lifecycle manager to support DT lifecycle phases. - The past invocations of DT lifecycle operations are tracked via a queue. This will eventually help with replay of operations on DT. - The invocations of lifecycle operations are completely asynchronous. It is possible to run all the phases at the same time. - The runner code is ready to be published as npm package. The README contains instructions on setting up private npm repository for development purposes. - FIX: There are no tests

- Adds skeleton invocation of ExecaCMDRunner from nestjs code - Formats code using prettier

- Sets the correct ts-jest preset for tests - Removes the incorrect npm registry setting in yarn.lock file

- Fixes eslint and jest configurations so that all yarn commands work correctly - Removes some of the lifeCycle code to make the basic code work

- Adds lifecycleManager and its dependencies - Fixes the yarn publish step. Now the package can be published successfully

codeclimate

The PR diff size of 7105 lines exceeds the maximum allowed for the inline comments feature.

codecov-commenter · 2023-09-15T14:26:46Z

Codecov Report

Merging #110 (088669a) into feature/distributed-demo (a95ad9c) will increase coverage by 1.42%.
Report is 3 commits behind head on feature/distributed-demo.
The diff coverage is 93.02%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@                     Coverage Diff                      @@
##           feature/distributed-demo     #110      +/-   ##
============================================================
+ Coverage                     64.37%   65.80%   +1.42%     
============================================================
  Files                            38       35       -3     
  Lines                           494      424      -70     
  Branches                         28       27       -1     
============================================================
- Hits                            318      279      -39     
+ Misses                          160      129      -31     
  Partials                         16       16

Files	Coverage Δ
servers/execution/runner/src/app.controller.ts	`100.00% <100.00%> (ø)`
servers/execution/runner/src/queue.service.ts	`100.00% <100.00%> (ø)`
...s/execution/runner/src/lifecycleManager.service.ts	`90.90% <90.90%> (ø)`
servers/execution/runner/src/execaCMDRunner.ts	`86.66% <86.66%> (ø)`

... and 7 files with indirect coverage changes

Components	Coverage Δ
Website	`62.72% <ø> (ø)`
Lib Microservice	`∅ <ø> (∅)`

prasadtalasila · 2023-09-16T06:20:13Z

@nichlaes quick questions:

~~If we use npx, can the cross-env be removed from script/*.bash?~~ NO
Is there a way to launch dist/src/runner.js on Windows as a standalone executable? It is a Javascript program but needs the following Nodejs arguments to work.
node -es-module-specifier-resolution=node --experimental-modules --experimental-specifier-resolution=node

Without cross-env, we get:

npx NODE_OPTIONS=--experimental-vm-modules \
  NODE_NO_WARNINGS=1 \
  jest --coverage
npm ERR! code EINVALIDTAGNAME
npm ERR! Invalid tag name "NODE_OPTIONS=--experimental-vm-modules" of package "NODE_OPTIONS=--experimental-vm-modules": Tags may not have any characters that encodeURIComponent encodes.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/prasad/.npm/_logs/2023-09-16T08_09_38_602Z-debug-0.log

- Completes integration tests for the codebase - Makes the execaCMDRunner catch the invalid commands - Removes get-stream dependency from execaCMDRunner - Uploads codecoverage to codecov - Adds new yarn command to watch the tests - Adds emojis to README

codeclimate

The PR diff size of 7253 lines exceeds the maximum allowed for the inline comments feature.

prasadtalasila · 2023-09-16T14:17:05Z

The two codeclimate issues are contradicting the coding idioms suggested by supertest. These two codeclimate issues need to be ignored.

- Add OpenAPI specification for DT Runner - Updates git-hooks to check against runner as well - Adds more yarn test commands to runner

codeclimate

The PR diff size of 7401 lines exceeds the maximum allowed for the inline comments feature.

nichlaes · 2023-09-19T07:42:48Z

@nichlaes quick questions:
2. Is there a way to launch dist/src/runner.js on Windows as a standalone executable? It is a Javascript program but needs the following Nodejs arguments to work.
node -es-module-specifier-resolution=node --experimental-modules --experimental-specifier-resolution=node

@prasadtalasila
As I understand it, we need to either use cross-env or enforce the use of bash when we are on Windows, in order for the passed environments to work.

prasadtalasila · 2023-09-19T07:50:18Z

We are currently restricted to Debian/Ubuntu. This is not a priority but the goal is to make cross-platform software.

Since the DT Runner is being developed from scratch now, I am exploring an easier way of adding cross-platform support.

More over, there is a definite requirement to execute DT Runner on Windows. The requirement will certainly come to us in a years' time.

prasadtalasila · 2023-09-19T08:30:13Z

Would it be better to put powershell scripts in script/ directory? Or is there a better solution?

nichlaes · 2023-09-19T10:30:16Z

Hmm maybe a possibility would be to build it as a single executable file on both platforms?

I not sure I have an understanding of what we want to do exactly, but maybe we could talk about it in person at some point?

nichlaes · 2023-09-19T11:54:13Z

@prasadtalasila If we put bash -c before running the scripts in the package.json, we can remove the cross-env. The only requirement is that the windows machine has bash installed.

"scripts": {
     . . . 
    "start": "bash -c 'script/start.bash'",
    "test": "bash -c './script/test.bash'",
     . . .

nichlaes · 2023-09-19T12:21:15Z

servers/execution/runner/script/start.bash

+npx cross-env \
+  NODE_OPTIONS="$MOD_RES $EXP $EXP_RES" \
+  NODE_NO_WARNINGS=1 \
+  node dist/src/main.js


Should it run 'main' or 'runner'?

It should run main.js.

The runner.ts was put in place to check the functionality of classes before tests were written. Also the
yarn publish uses dist/runner.js. That need to be changed to dist/main.js.

I will change this and check again.

nichlaes · 2023-09-19T13:07:21Z

Here are some of my comments on the PR:

It took me some time to grasp the whole 'phase' concept. Of course I'm still very new to the project, so maybe that is just how it is. You could consider if it would make sense to have it mention a bit further in the README or just an example.

Another comment, which properly out of scope / nice-to-have. As it is now, it would not be possible to see the logs before the processes has succeeded/failed. Therefore we can't give a continuously status back to the user. I could imagine, this would be nice for the user, if the 'phase' takes a longer amount of time.

Otherwise everything looks good. The code coverage is high, and the parts that are not covered are trivial.

prasadtalasila · 2023-09-19T13:59:49Z

@prasadtalasila If we put bash -c before running the scripts in the package.json, we can remove the cross-env. The only requirement is that the windows machine has bash installed.
"scripts": {
     . . . 
    "start": "bash -c 'script/start.bash'",
    "test": "bash -c './script/test.bash'",
     . . . 

@nichlaes
Interesting. I will explore this possibility.

prasadtalasila · 2023-09-19T14:01:14Z

Hmm maybe a possibility would be to build it as a single executable file on both platforms?

I not sure I have an understanding of what we want to do exactly, but maybe we could talk about it in person at some point?

pkg or its equivalent software will do it. Certainly worth exploring. Thanks for the suggestion.

prasadtalasila · 2023-09-19T14:15:16Z

Here are some of my comments on the PR:

It took me some time to grasp the whole 'phase' concept. Of course I'm still very new to the project, so maybe that is just how it is. You could consider if it would make sense to have it mention a bit further in the README or just an example.

This is a relevant comment. The concept is explained in https://into-cps-association.github.io/DTaaS/version0.2/user/digital-twins/lifecycle.html (documentation).

It must also be explained here as well. I will add more details in the README.

Another comment, which properly out of scope / nice-to-have. As it is now, it would not be possible to see the logs before the processes has succeeded/failed. Therefore we can't give a continuously status back to the user. I could imagine, this would be nice for the user, if the 'phase' takes a longer amount of time.

I need to change the stdout and stderr from string to streams.
Will come in future PRs.

Otherwise everything looks good. The code coverage is high, and the parts that are not covered are trivial.

Ok. Thanks for the review.

prasadtalasila · 2023-09-20T07:55:28Z

@prasadtalasila If we put bash -c before running the scripts in the package.json, we can remove the cross-env. The only requirement is that the windows machine has bash installed.
"scripts": {
     . . . 
    "start": "bash -c 'script/start.bash'",
    "test": "bash -c './script/test.bash'",
     . . . 

The problem with bash -c is that it requires Windows Subsystem for Linux. This is not ideal. Can we use powershell scripts in Windows as equivalents for bash scripts? What are the downsides?

codeclimate

The PR diff size of 7479 lines exceeds the maximum allowed for the inline comments feature.

codeclimate · 2023-09-27T13:05:53Z

Code Climate has analyzed commit 088669a and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Duplication	2

View more on Code Climate.

prasadtalasila and others added 8 commits September 15, 2023 10:10

A minimum working example for the configuration files

c0d3e84

Refactors github actions

e14224b

Adds skeleton code for nestjs framework

e65c053

Combines command runner code with nestjs code

86890e4

- Adds skeleton invocation of ExecaCMDRunner from nestjs code - Formats code using prettier

Fix tests for DT runner (#100)

6b6c4e7

- Sets the correct ts-jest preset for tests - Removes the incorrect npm registry setting in yarn.lock file

Fixes configuration and tests

05e635a

- Fixes eslint and jest configurations so that all yarn commands work correctly - Removes some of the lifeCycle code to make the basic code work

Updates server routes and service functionality

f28f400

- Adds lifecycleManager and its dependencies - Fixes the yarn publish step. Now the package can be published successfully

codeclimate bot reviewed Sep 15, 2023

View reviewed changes

prasadtalasila requested a review from nichlaes September 15, 2023 14:25

Completes integration tests

d31da4a

- Completes integration tests for the codebase - Makes the execaCMDRunner catch the invalid commands - Removes get-stream dependency from execaCMDRunner - Uploads codecoverage to codecov - Adds new yarn command to watch the tests - Adds emojis to README

codeclimate bot reviewed Sep 16, 2023

View reviewed changes

Adds API Specification for DT Runner and updates shell scripts

2fe0344

- Add OpenAPI specification for DT Runner - Updates git-hooks to check against runner as well - Adds more yarn test commands to runner

codeclimate bot reviewed Sep 18, 2023

View reviewed changes

nichlaes reviewed Sep 19, 2023

View reviewed changes

prasadtalasila added 2 commits September 20, 2023 13:38

Update launch script for the package

f86642d

Improves README

3be8da0

Corrects docs/ publish instructions

088669a

codeclimate bot reviewed Sep 27, 2023

View reviewed changes

prasadtalasila merged commit 6e6b9f5 into INTO-CPS-Association:feature/distributed-demo Sep 27, 2023
3 checks passed

prasadtalasila deleted the runner-nestjs branch September 27, 2023 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Digital Twin Runner utility #110

Adds Digital Twin Runner utility #110

prasadtalasila commented Sep 15, 2023 •

edited

Loading

codeclimate bot left a comment

codecov-commenter commented Sep 15, 2023 •

edited

Loading

prasadtalasila commented Sep 16, 2023 •

edited

Loading

codeclimate bot left a comment

prasadtalasila commented Sep 16, 2023

codeclimate bot left a comment

nichlaes commented Sep 19, 2023

prasadtalasila commented Sep 19, 2023

prasadtalasila commented Sep 19, 2023

nichlaes commented Sep 19, 2023

nichlaes commented Sep 19, 2023

nichlaes Sep 19, 2023

prasadtalasila Sep 19, 2023 •

edited

Loading

nichlaes commented Sep 19, 2023

prasadtalasila commented Sep 19, 2023

prasadtalasila commented Sep 19, 2023 •

edited

Loading

prasadtalasila commented Sep 19, 2023

prasadtalasila commented Sep 20, 2023

codeclimate bot left a comment

codeclimate bot commented Sep 27, 2023

Adds Digital Twin Runner utility #110

Adds Digital Twin Runner utility #110

Conversation

prasadtalasila commented Sep 15, 2023 • edited Loading

codeclimate bot left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 15, 2023 • edited Loading

Codecov Report

prasadtalasila commented Sep 16, 2023 • edited Loading

codeclimate bot left a comment

Choose a reason for hiding this comment

prasadtalasila commented Sep 16, 2023

codeclimate bot left a comment

Choose a reason for hiding this comment

nichlaes commented Sep 19, 2023

prasadtalasila commented Sep 19, 2023

prasadtalasila commented Sep 19, 2023

nichlaes commented Sep 19, 2023

nichlaes commented Sep 19, 2023

nichlaes Sep 19, 2023

Choose a reason for hiding this comment

prasadtalasila Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

nichlaes commented Sep 19, 2023

prasadtalasila commented Sep 19, 2023

prasadtalasila commented Sep 19, 2023 • edited Loading

prasadtalasila commented Sep 19, 2023

prasadtalasila commented Sep 20, 2023

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot commented Sep 27, 2023

prasadtalasila commented Sep 15, 2023 •

edited

Loading

codecov-commenter commented Sep 15, 2023 •

edited

Loading

prasadtalasila commented Sep 16, 2023 •

edited

Loading

prasadtalasila Sep 19, 2023 •

edited

Loading

prasadtalasila commented Sep 19, 2023 •

edited

Loading