Feature/model service

[janniclas]

This pull request is made based on the results of #83 so please only merge after this was merged.

This pull request introduces a model service and a store service as an additional abstraction layer. The model layer is used to subscribe to any changes made to the state stored in the store service. This is used to share the information about certain instances with multiple components without making the same server request all over again.

If this, admittedly pretty large request, needs further clarification don't hesitate to ask :)

This pretty much closes #53

[johannesduesing]

As requested by @janniclas i did a functionality based test on this PR. These are my results:

• General communication with instance registry 0.8.0 works
• Number of Components in the dashboard is displayed correctly, and updates without reloading the page
• Adding an instance works
• Table rows for component types updated without reloading (on add instance / delete instance via curl)
• Instance attributes do not update without reloading, meaning that state does not change without hitting Reload (is this intended for this PR?)
• Docker controls are displayed for non-docker instances (is this intended for this PR?)
• Docker controls do not work, no start/stop/pause/resume/delete calls to the registry are being made (is this intended for this PR?)

[janniclas]

thx for the additional inquiry @johannesduesing .
In my tests the docker control requests end up at the ir but then I get a docker host error like shown on thursday.
@ishwaryaPaderborn what is your behavior in this case?

state should be reloading I will have a look at that.
Regarding the docker controls, there is an issue open for this, which will be addressed in a future update

EDIT: state should not be reloading automatically sorry my mistake. It should and will in the long run, but this update doesn't contain the functionality to handle the state change events of the instance registry right now. They are only included in another branch on which I'm working, got confused there.

[johannesduesing]

That is very strange. I did another test, just to make sure. As stated before, adding an instance from the UI works just fine, reaches the registry and is executed. The added instance shows up in the table, but i can not use the docker controls for start / stop / pause / resume / delete, because they do not reach the registry.
I opened the developer console and found the following messages when clicking the docker controls:

Object { headers: {…}, status: 400, statusText: "Bad Request", url: "http://localhost:8083/api/resumeInstance?instanceID=a1", ok: false, name: "HttpErrorResponse", message: "Http failure response for http://localhost:8083/api/resumeInstance?instanceID=a1: 400 Bad Request", error: null }

The same goes for stopInstance etc. Could the problem be related to the fact that it says instanceID=a1 in the URL query parameter ? Or is the character 'a' really needed here ?

[ishwaryamamidi]

@janniclas as per your suggestion I have cloned all the repositories newly and tested. Below is the result:

1. Able to get the Instances and the Running Instances number in the dashboard
2. Able to 'Add' an Instance for any type of component
3. But unable to 'Stop' or 'Pause' any running Instance (Because this dint work could not test'Delete' an Instance functionality as only 'Stopped' Instance can be deleted)

@johannesduesing Yes, I also got 404 error for 'Stop' and 'Pause' functionalities.
I'm not sure if this is because of appending an 'a' with the InstanceId while sending a request, because it was working fine when we tested it a month before. This appending 'a' was implemented by @winniedo because she was facing some issue for sending the 'Instance Id' just as a number.

@winniedo could you please confirm the exact reason for appending 'a' to the 'Insatnce Id'?

[ishwaryamamidi]

@johannesduesing yes, your guess was correct. :) The probem was because of the 'a'.

I just now tested by removing the 'a' while sending the request to ir for 'start/Stop/play/resume' and the docker functionality works as expected. None of the existing functionalities is broken in the latest test.
Hence, I will raise a new pull request to correct this issue.

[codecov-io]

Codecov Report

Merging #89 into develop will decrease coverage by 0.29%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           develop     #89     +/-   ##
=========================================
- Coverage     4.95%   4.65%   -0.3%     
=========================================
  Files           11      11             
  Lines          202     215     +13     
  Branches        11       9      -2     
=========================================
  Hits            10      10             
- Misses         192     205     +13

Impacted Files	Coverage Δ
app/controllers/InstanceRegistryController.scala	0% <0%> (ø)	⬆️
app/controllers/ApiRouter.scala	0% <0%> (ø)	⬆️
app/models/Instance.scala	0% <0%> (ø)	⬆️
app/models/Event.scala	0% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f117a50...905426a. Read the comment docs.

[janniclas]

@johannesduesing @ishwaryaPaderborn thx for your review and the catch with the invalid url.
I've merged ishwarya's fix into the develop and also in this branch, so please verify if this branch now has the intended behavior.

[johannesduesing]

I checked out the latest version and found the following:

• After adding an instance (which still works) the initial state (Deploying) is replaced by Running without reloading, so as expected
• Start / Stop / Pause / Resume now reach the registry and are being executed as expected. Still they result in errors in the console (see error message below). These state changes are not updated without reloading, only the initial change from Deploying to Running works without reloading.

Object { headers: {…}, status: 200, statusText: "OK", url: "http://localhost:8083/api/stopInstance?instanceID=1", ok: false, name: "HttpErrorResponse", message: "Http failure during parsing for http://localhost:8083/api/stopInstance?instanceID=1", error: {…} }

• The Delete button does not seem to do anything. Does not work when state is Stopped, does not work for any other state, and triggers no messages in the console

EDIT:
This is the full error description from the console:

{…}
​
error: {…}
​​
error: SyntaxError: "JSON.parse: unexpected character at line 1 column 1 of the JSON data"
	onLoad http://localhost:8083/vendor.js:32409:46
	invokeTask http://localhost:8083/polyfills.js:2766:17
	onInvokeTask http://localhost:8083/vendor.js:76418:24
	invokeTask http://localhost:8083/polyfills.js:2765:17
	runTask http://localhost:8083/polyfills.js:2533:28
	invokeTask http://localhost:8083/polyfills.js:2841:24
	invokeTask http://localhost:8083/polyfills.js:3885:9
	globalZoneAwareCallback http://localhost:8083/polyfills.js:3911:17
​​
text: "Operation accepted."
​​
<prototype>: Object { … }
​
headers: Object { normalizedNames: Map(0), lazyUpdate: null, lazyInit: lazyInit()
 }
​
message: "Http failure during parsing for http://localhost:8083/api/pauseInstance?instanceID=1"
​
name: "HttpErrorResponse"
​
ok: false
​
status: 200
​
statusText: "OK"
​
url: "http://localhost:8083/api/pauseInstance?instanceID=1"
​
<prototype>: Object { constructor: HttpErrorResponse()
 }

[ishwaryamamidi]

Tested the updated code and found few results:
As reported by @johannesduesing I also found the same result for Adding and Start/Stop/Play/Resume functionalities.
But I was successfully able to 'delete' the added instance. Please find the attached screenshots for 'Delete' functionality

[janniclas]

since the observed behavior of both reviewers differ we will postpone this till thursday's meeting where we can hopefully sort things out together.
thx for your work so far @ishwaryaPaderborn and @johannesduesing

[johannesduesing]

Okey this is now hopefully my final contribution to the testing for this PR 😄
I found the following:

Firefox 64.0.2 on Ubuntu 16.04 LTS 64-bit

• Delete button does not work, triggers no dialog box and no messages in the console

Google Chrome 71.0.3578.98 on Windows 7 Ultimate 64-bit

• Delete button triggers dialog box as expected
• Confirm deletes instance as expected if instance was stopped, otherwise shows warning that deletion is not possible
• Cancel does also trigger deletion!! If you click Delete->Cancel, a delete call is issued to registry! If the instance was stopped this means it will be deleted even though the user clicked Cancel! In my opinion, this must be fixed asap.

I don't know why it works in Chrome and not in firefox.. Maybe you have an idea @janniclas ?

EDIT:
I found the issue with the Cancel button in table-all.component.ts line 72-88:

 dialogRef.afterClosed().subscribe(result => {
            console.log('delete state', instance.instanceState);
            if (result === 'Confirm' && instance.instanceState === 'Running') {
                console.log('alert working');
                alert('Please Stop the Instance before you try to delete');
                console.log('data', this.dataSource.data);

            } else {
                this.apiService.deleteInstance(id).subscribe((deleteResult: HttpEvent<number>) => {
                    console.log('result', deleteResult);
                }, err => {
                    console.log('error delete Instance');
                });
                this.removeAt(i);
            }
            this.dialogResult = result;
        });

Here you only check for result Confirm & State Running, there you show a warning (as expected), but for all other cases you just execute the delete call. You never check for result Cancle here.

[johannesduesing]

Update regarding the latest changes:

• Delete dialog now works as expected in Firefox
• Cancel button works as expected.

[johannesduesing]

To me the only problem left with this PR now is the parsing error when clicking the docker controls, and the result is that the state is not updating without reloading the page. But since this is known to @janniclas i think this PR is ready to merge.

[janniclas]

since the remaining bug has nothing to do with the functionality added with this pull request it will be merged now and a ticket opened for the bug.