Skip to content

REST JavaScript client#165

Merged
kitchoi merged 17 commits into
masterfrom
140-rest-js-client
Jul 27, 2016
Merged

REST JavaScript client#165
kitchoi merged 17 commits into
masterfrom
140-rest-js-client

Conversation

@stefanoborini

Copy link
Copy Markdown
Contributor

Uses the REST interface to start and stop containers, removing the need for the POST method in HomeHandler.

@stefanoborini

Copy link
Copy Markdown
Contributor Author

Fixes #140

@codecov-io

codecov-io commented Jul 26, 2016

Copy link
Copy Markdown

Current coverage is 87.24% (diff: 86.66%)

Merging #165 into master will increase coverage by 2.11%

@@             master       #165   diff @@
==========================================
  Files            36         37     +1   
  Lines          1379       1294    -85   
  Methods           0          0          
  Messages          0          0          
  Branches        130        120    -10   
==========================================
- Hits           1174       1129    -45   
+ Misses          166        126    -40   
  Partials         39         39          

Powered by Codecov. Last update 728f540...79c595c

from remoteappmanager.rest.resource import Resource
from remoteappmanager.docker.container import Container as DockerContainer
from remoteappmanager.utils import url_path_join
from remoteappmanager.netutils import wait_for_http_server_2xx

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could wait_for_http_server_2xx be placed in utils?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer it in a separate module that is aimed at network.

@kitchoi

kitchoi commented Jul 27, 2016

Copy link
Copy Markdown
Contributor

Note that in the HomeHandler on master, if 'wait_for_http_server_2xx' throws a Timeout error, an error message is displayed and the container is removed. It is no longer the case here, instead the start button "loads" forever and the started container is not removed.

Original behaviour (tried to start 'novnc-never-starts':
screenshot from 2016-07-27 11 05 20

Behaviour for this PR:
screenshot from 2016-07-27 10 54 59

Comment thread tests/test_netutils.py

@gen_test
def test_basic(self):
yield wait_for_http_server_2xx(self.get_url("/short"), timeout=2)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how to make sure the request would finish within 2 seconds?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind that, the computer has to be reaaaaally slow to not have completed the request.

@kitchoi

kitchoi commented Jul 27, 2016

Copy link
Copy Markdown
Contributor

There is a retrogression on the error handling (see above), but I think we can fix that on a separate PR, all within release 0.6.0? The rest looks good to merge to me

@stefanoborini

Copy link
Copy Markdown
Contributor Author

@kitchoi No I fix it here.

@stefanoborini

Copy link
Copy Markdown
Contributor Author

Ok, the reason why you are not seeing the error is because we are missing bootstrap. That produces an error in the javascript execution.

In any case, I ported the server side error handling (so it behaves like the old code) but I also need to introduce the javascript error handling to remove the spinner.

@kitchoi

kitchoi commented Jul 27, 2016

Copy link
Copy Markdown
Contributor

👍

@kitchoi kitchoi merged commit 6853584 into master Jul 27, 2016
@kitchoi kitchoi deleted the 140-rest-js-client branch July 27, 2016 15:19
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.

3 participants