fix nested executions using non-standard ports #1389

Merged
merged 3 commits into from Apr 7, 2016

Projects

None yet

3 participants

@HarryWeppner
Contributor

Nested executions using non-default ports did not work as env.port was prioritized over hostname and ssh config derived port numbers.

See test_tasks.py:TestExecute.test_nested_execution_with_explicit_ports

@HarryWeppner
Contributor

Hi there,

do you have an eta when you'll be able to review and merge this pull request?

Thanks & cheerio, Harry.

@bitprophet
Member

I'm not comfortable making this type of change at this point in 1.x's lifecycle (see http://www.fabfile.org/roadmap.html) - the chance for breaking existing-known-to-work functionality feels a bit too high :( thanks, and sorry!

@bitprophet bitprophet closed this Oct 26, 2015
@HarryWeppner
Contributor

Hi,

that's a bummer because I

  • tried to keep code changes to an absolute minimum
  • avoided larger refactoring to stay within the existing style
  • made sure it passes all existing tests
  • added a new test to avoid future regressions

The only aspect this commit disambiguates is the priority of port information, which is reflected in a comment.

Working around this bug is painful and so is adding a custom patch for vanilla installations.

I'd appreciate it if you could reconsider and review the changes once more. On my end, I'll promise to find time to contribute to the 2.0 roadmap.

Thanks & cheerio, Harry.

@bitprophet
Member

Thank you for taking the time for a reasoned & calm response, I appreciate it!

To expand my initial reply, I'm not knocking the quality of your work or the caution you took (again, it's appreciated!). However, the history of code (or at least, all projects I've maintained) is rife with well-meaning, well-tested, small changes which nonetheless broke previously unseen/untested use cases. The more critical the code path (network.connect is as critical as it gets), the more of those pitfalls come into play.

So every bugfix is a risk assessment balancing act of "how many people are encountering this bug?" (I haven't seen many other reports that seem to map to this) vs "what's the chance of this bugfix causing other bugs?" (higher than I'd like).

Compound that with the previously-noted conservative approach that I'm applying to Fabric 1 as 2 looms, and you get my original "ehh this changes how connect() handles port numbers, that's scary" reaction.


All that said, maintainership is a constant battle between "this could break shit!" and "people can always just roll back!" and I'm trying to cut more, smaller releases - meaning the cost of rollback should be lower than it might otherwise. So I will think about this some more and make another judgement call soon. Thanks again.

@bitprophet
Member

Upon reflection, let's do it after all. Thanks @HarryWeppner. Adding to release milestone (feature release, because of potential impact/risk - this is a common practice :))

@bitprophet bitprophet reopened this Nov 10, 2015
@bitprophet bitprophet added this to the 1.11 milestone Nov 10, 2015
@HarryWeppner
Contributor

great - thanks for reconsidering! In the (unlikely) event it breaks something I'll be around to help...

HarryWeppner added some commits Nov 13, 2015
@HarryWeppner HarryWeppner fix escape sequences in open_shell
In the context of open_shell with sys.stdin connected to a channel,
cursor movements appear "wonky", which can really mess up the user
experience.

An initial escape sequence is not captured correctly and only partially
sent across the channel resulting in no effect. A repeated key press
yields the correct sequence.

I suspect this is caused by a mismatch between select (mapped to a
syscall) and reading from the (buffered) sys.stdin. Dropping to the
lower-level os.read resolves this issue entirely.

NOTE: this may explain fabric#196
8efbff1
@HarryWeppner HarryWeppner Revert "fix escape sequences in open_shell"
sorry, wrong branch

This reverts commit 8efbff1.
ac4762f
@HarryWeppner
Contributor

Sorry, accidentally published a commit for a unrelated pull request here, which has been separated to pull request #1398 .

@kaaveland kaaveland commented on the diff Jan 22, 2016
fabric/network.py
if omit_port:
return user, host
+
+ # determine port from ssh config if enabled
+ ssh_config_port = None
+ if env.use_ssh_config:
+ ssh_config_port = conf.get('port', None)
+
+ # port priorities (from highest to lowest)
@kaaveland
kaaveland Jan 22, 2016

This sort of thing seems to me as something that should be documented somewhere. Maybe something like this should go into the docstring here?

@kaaveland kaaveland commented on the diff Jan 22, 2016
tests/test_tasks.py
@@ -417,6 +417,47 @@ class MyTask(Task):
mytask = MyTask()
execute(mytask)
+ @server(port=2200)
+ @server(port=2201)
+ def test_nested_execution_with_explicit_ports(self):
+ """
+ nested executions should work with defined ports
+ """
+
+ def sub_task_one():
@kaaveland
kaaveland Jan 22, 2016

Matter of personal taste I guess, but I think the main task would read more clearly if this was named something like assert_port_is_2201

@bitprophet bitprophet merged commit ac4762f into fabric:master Apr 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bitprophet bitprophet added a commit that referenced this pull request Apr 7, 2016
@bitprophet bitprophet Changelog re #1389 63a58f0
@bitprophet
Member

Merged, including addressing Robin's concerns. Thanks both!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment