Skip to content

Send connection upgrade headers#1124

Closed
dgageot wants to merge 2 commits intodocker:masterfrom
dgageot:fix-1123
Closed

Send connection upgrade headers#1124
dgageot wants to merge 2 commits intodocker:masterfrom
dgageot:fix-1123

Conversation

@dgageot
Copy link
Copy Markdown
Member

@dgageot dgageot commented Jul 8, 2016 •
edited
Loading

Fixes #1123

I'm all new to this codebase, so I'm not sure I did the things right.

I basically looked at docker cli code and found out where the postHijacked function is called:

I tried to add a unit test for attach but couldn't find any to get inspiration from.

@dgageot dgageot mentioned this pull request Jul 8, 2016
Closed
@bfirsh
Copy link
Copy Markdown
Contributor

bfirsh commented Jul 8, 2016

Looks like this is sending it as part of the request payload rather than as header. You probably want to pass it as a headers argument to _post_json.

@dgageot
Copy link
Copy Markdown
Member Author

dgageot commented Jul 8, 2016

@bfirsh sure...

dgageot added 2 commits July 8, 2016 17:25
@dgageot
Add hijack hints for attach api calls
29faf56 
Signed-off-by: David Gageot <david@gageot.net>
@dgageot
Add hijack hints for non-detached exec api calls
2ed97bb 
Signed-off-by: David Gageot <david@gageot.net>
@dgageot
Copy link
Copy Markdown
Member Author

dgageot commented Jul 8, 2016

@bfirsh should be better now

@shin-
Copy link
Copy Markdown
Contributor

shin- commented Jul 8, 2016

I suppose we need some client-side code to handle the change of protocol?

The Connection Aborted issue only happens when we're executing the tests from inside a container, this is what I get running locally.

$ py.test tests/integration/exec_test.py 
============================= test session starts ==============================
platform linux2 -- Python 2.7.9 -- py-1.4.30 -- pytest-2.7.2
rootdir: /home/joffrey/work/pydocker, inifile: pytest.ini
plugins: cov
collected 7 items 

tests/integration/exec_test.py FFFF..F

=================================== FAILURES ===================================
______________________ ExecTest.test_exec_command_as_root ______________________
tests/integration/exec_test.py:74: in test_exec_command_as_root
    self.assertEqual(exec_log, b'root\n')
E   AssertionError: '' != 'root\n'
______________________ ExecTest.test_exec_command_as_user ______________________
tests/integration/exec_test.py:58: in test_exec_command_as_user
    self.assertEqual(exec_log, b'default\n')
E   AssertionError: '' != 'default\n'
_____________________ ExecTest.test_exec_command_streaming _____________________
tests/integration/exec_test.py:92: in test_exec_command_streaming
    self.assertEqual(res, b'hello\nworld\n')
E   AssertionError: '' != 'hello\nworld\n'
______________________ ExecTest.test_exec_command_string _______________________
tests/integration/exec_test.py:42: in test_exec_command_string
    self.assertEqual(exec_log, b'hello world\n')
E   AssertionError: '' != 'hello world\n'
________________________ ExecTest.test_execute_command _________________________
tests/integration/exec_test.py:26: in test_execute_command
    self.assertEqual(s, b'hello\n')
E   AssertionError: '' != 'hello\n'

@shin- shin- mentioned this pull request Jul 8, 2016
Closed
@dgageot
Copy link
Copy Markdown
Member Author

dgageot commented Jul 9, 2016

Closing in favor of #1126

@dgageot dgageot closed this Jul 9, 2016
@aanand aanand mentioned this pull request Jul 13, 2016
Merged
@shin- shin- added this to the 1.10.0 milestone Jul 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

status/0-triage

Projects

None yet

Milestone

1.10.0

Development

Successfully merging this pull request may close these issues.

4 participants

@dgageot @bfirsh @shin- @GordonTheTurtle