Scottx611x/read input from url #20

scottx611x · 2018-04-12T15:34:09Z

No description provided.

…ontainers with `INPUT_JSON_URL`

…sn't around anymore

scottx611x · 2018-04-12T15:34:36Z

build_run_test.sh

+# Returns a url that will resolve to the SimpleHTTPServer we spin up below 
+# so that the Docker container can make requests to files existing 
+# under the cwd i.e. `./test-data/input.json`
+get_python_server_url() {


@mccalluc I feel bad about using this again, but it works...

We can't do this. it might be catching edge cases we're unaware of, but if it did break, we're not in a place to understand it's behavior. There's a grep example earlier in that thread, or I could find perl readable here:

ifconfig | perl -ne 'print $2 if /inet (addr:)?(\d+\.\d+\.\d+\.\d+)/ and $2 ne "127.0.0.1"'

Or move this whole script into python?

mccalluc · 2018-04-12T22:30:12Z

build_run_test.sh

+
+# Spin up a server so that the container can GET input data from the `test-data` dir
+python -m SimpleHTTPServer $PYTHON_SERVER_PORT &
+PYTHON_SERVER_PID=$!

 for FIXTURE in `ls input_fixtures`; do


just glob instead of subprocess?:

for FIXTURE in input_fixtures/*; do

mccalluc · 2018-04-12T22:31:36Z

build_run_test.sh

+python test.py
+
+kill $PYTHON_SERVER_PID


Unless you nohup, shouldn't the child exit with the parent?

mccalluc · 2018-04-12T22:34:34Z

context/on_startup.py

 from cgi import escape


+def get_refinery_input():
+    """ Make a GET request to acquire the input data for the container"""
+    return requests.get(os.environ["INPUT_JSON_URL"]).json()


But isn't refinery using using json in INPUT_JSON? I could be confused.

INPUT_JSON_URL is what refinery is passing along to containers after: refinery-platform/refinery-platform#2677

mccalluc · 2018-04-12T22:35:47Z

context/on_startup.py

+            status = requests.get(
+                url, headers={
+                    'Range': 'bytes=0-0'}).status_code
+        except requests.exceptions.RequestException as e:


weird line breaks...

mccalluc · 2018-04-12T22:37:23Z

build_run_test.sh

+}
+
+# Spin up a server so that the container can GET input data from the `test-data` dir
+python -m SimpleHTTPServer $PYTHON_SERVER_PORT &


http.server for python3?

mccalluc · 2018-04-12T22:38:41Z

test.py



 class ContainerTest(unittest.TestCase):

    def get_url(self, name):
        command = "docker port {} | perl -pne 's/.*://'".format(name)
-        port = subprocess.check_output(command, shell=True).strip().decode('utf-8')
+        port = subprocess.check_output(
+            command, shell=True).strip().decode('utf-8')


I would put another linebreak after True to respect structure.

mccalluc · 2018-04-12T22:41:18Z

test.py

-        self.assertIn(expected, response.text)
+        # TODO: Not ideal for error pages
+        self.assertEqual(200, response.status_code)
+        if expected is not None:


why do we need this? Stricter tests are usually better.

mccalluc · 2018-04-12T22:41:51Z

test.py

-    def test_data_directory(self):
-        self.assert_expected_response('good', '{', '/data/input.json')
+    def test_input_data_url(self):
+        self.assert_expected_response('good', path='/options.json')


why no expected?

mccalluc · 2018-04-12T22:43:25Z

test.py

                print '{} -> {}'.format(name, url)
                return url
            print('Still waiting for server...')
            time.sleep(1)
        self.fail('Server never came up: ' + name)

-    def assert_expected_response(self, name, expected, path='/'):
+    def assert_expected_response(self, name, expected=None, path='/'):


You can still using positional params when a default is given...

mccalluc

Lots of little things...

…get around the SSL InsecurePlatformWarning errors

…8449 (comment)

scottx611x · 2018-04-17T13:47:23Z

context/on_startup.py

@@ -61,7 +71,7 @@ def validate_urls(urls):
        if status != 206  # If byte-ranges are handled, should be 206, not 200.
    ]
    if messages:
-        raise StandardError('\n'.join(messages))
+        raise Exception('\n'.join(messages))


back to StandardError? Or move to Python3

scottx611x · 2018-04-17T13:47:30Z

context/on_startup.py

@@ -72,9 +82,10 @@ def start_server():
 if __name__ == '__main__':
    try:
        write_igv_configuration()
-    except StandardError, e:
+    except Exception as e:


back to StandardError? Or move to Python3

scottx611x · 2018-04-17T13:48:21Z

define_repo.sh

@@ -2,4 +2,5 @@

 OWNER=gehlenborglab
 NAME=docker_igv_js
+export NAME=$NAME


export NAME=docker_igv_js

scottx611x · 2018-04-17T13:49:31Z

test.py

        url = 'http://localhost:{}'.format(port)
        for i in xrange(5):
-            if 0 == subprocess.call('curl --fail --silent ' + url + ' > /dev/null', shell=True):


This was updated to requests somewhee, use that code

scottx611x · 2018-04-17T13:50:25Z

test.py

-import requests
+import unittest
+
+from test_utils import TestContainerRunner


 class ContainerTest(unittest.TestCase):

    def get_url(self, name):
        command = "docker port {} | perl -pne 's/.*://'".format(name)


double check docker-py functionality for getting port

scottx611x · 2018-04-17T13:53:38Z

test.py

maybe look into bumping this out

scottx611x · 2018-04-17T13:53:46Z

test.py

@@ -55,10 +67,14 @@ def test_no_parameters(self):


 if __name__ == '__main__':
+    test_container_runner = TestContainerRunner()


as a context manager?

scottx611x · 2018-04-17T14:01:32Z

test_utils.py

+            self.containers.append(container)
+
+    def cleanup_containers(self):
+        map(


for container in containers:
container remove

scottx611x

Fix these, please

scottx611x added 7 commits April 12, 2018 11:30

Serve directories containing existing input fixtures, and pass into c…

447d957

…ontainers with `INPUT_JSON_URL`

Add function to fetch input data from INPut_JSON_URL on startup

182b38f

Update paths since theres not /data fir anymore

f15270d

PEP8-ification

9e1ab1e

Change test to assert that options.json gets created since data dir i…

1f11f77

…sn't around anymore

Update existing tests to use kwargs for readability

6810427

PEP8 fixes

8495507

scottx611x requested a review from mccalluc April 12, 2018 15:34

scottx611x commented Apr 12, 2018

View reviewed changes

mccalluc reviewed Apr 12, 2018

View reviewed changes

build_run_test.sh Outdated

python test.py

kill $PYTHON_SERVER_PID

Copy link

Member

mccalluc Apr 12, 2018

Choose a reason for hiding this comment

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

Unless you nohup, shouldn't the child exit with the parent?

mccalluc reviewed Apr 12, 2018

View reviewed changes

scottx611x added 11 commits April 16, 2018 11:39

Move shell script functionality into python

2363879

Fix line breaks

542e9a2

Fix another linebreak

00601dd

Try out no sudo with pip

313eab0

Travis base python is 2.7.6; I think this should grab a newer v. and …

d2f21d5

…get around the SSL InsecurePlatformWarning errors

Specify minor python version

ffa01f6

Try the reccommended --user flag for pip; See: travis-ci/travis-ci#…

1053cba

…8449 (comment)

Specify a newer python version

9528b1a

Try upgrading pip

9e55db2

Specify python version and pip w/o sudo

6481471

Try removing sudo

4342e5a

scottx611x added 3 commits April 16, 2018 13:02

Simplify logic for expected condition

6ab74e8

Add expected text to assertion

c0f0620

A positional arg hereis clearer than having a default of None

998c2bf

scottx611x requested a review from mccalluc April 16, 2018 17:05

Remove kwarg uasage

4ec7ba9

scottx611x commented Apr 17, 2018

View reviewed changes

test.py Outdated

test_container_runner.cleanup_containers()

Copy link

Member Author

scottx611x Apr 17, 2018

Choose a reason for hiding this comment

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

maybe look into bumping this out

scottx611x commented Apr 17, 2018

View reviewed changes

scottx611x added 7 commits April 17, 2018 10:44

Use TestContainerRunner as a context manager

7b30ba8

Update to python 3

5a32596

Remove print

fa0e716

Use a simple for loop sytnax

fc2be9a

Export on one line

fd4e8b9

Don't use subprocess to curl; requests is a thing

85e8b73

Use docker functionality to get port info

0e96c45

scottx611x merged commit bbc33bf into master Apr 17, 2018

scottx611x deleted the scottx611x/read_input_from_url branch April 17, 2018 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scottx611x/read input from url #20

Scottx611x/read input from url #20

scottx611x commented Apr 12, 2018

scottx611x Apr 12, 2018 •

edited

mccalluc Apr 12, 2018

mccalluc Apr 12, 2018

mccalluc Apr 12, 2018

mccalluc Apr 12, 2018

mccalluc Apr 12, 2018

scottx611x Apr 16, 2018 •

edited

mccalluc Apr 12, 2018

mccalluc Apr 12, 2018

mccalluc Apr 12, 2018

mccalluc Apr 12, 2018

mccalluc Apr 12, 2018

mccalluc Apr 12, 2018

mccalluc left a comment

scottx611x Apr 17, 2018

scottx611x Apr 17, 2018

scottx611x Apr 17, 2018

scottx611x Apr 17, 2018

scottx611x Apr 17, 2018

scottx611x Apr 17, 2018

scottx611x Apr 17, 2018

scottx611x Apr 17, 2018

scottx611x left a comment

		@@ -55,10 +67,14 @@ def test_no_parameters(self):


		if __name__ == '__main__':
		test_container_runner = TestContainerRunner()

Scottx611x/read input from url #20

Scottx611x/read input from url #20

Conversation

scottx611x commented Apr 12, 2018

scottx611x Apr 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottx611x Apr 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccalluc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottx611x left a comment

Choose a reason for hiding this comment

scottx611x Apr 12, 2018 •

edited

scottx611x Apr 16, 2018 •

edited