contrib.files.contains() should not return combined stdout and stderr #345

Closed
bitprophet opened this Issue Aug 19, 2011 · 3 comments

Comments

Projects
None yet
1 participant
@bitprophet
Member

bitprophet commented Aug 19, 2011

Description

contrib.files.contains() returns output of 'egrep' command as a result. If you have hostname misconfigured sudo can put following line to 'stderr':

sudo: unable to resolve host hosname

Even if 'succeeded' flag is 'False' the result will be evaluated as 'True' because combined 'stdout' and 'stderr' is not empty.

This approach affects also 'append()' function.

One solution would be to return just stdout instead of combined streams but I failed to achieve that.

Other solution just uses succeeded flag as a result of contains(). My fix is here.


Originally submitted by Szymon Reichmann (drapichrust) on 2011-04-27 at 05:01am EDT

Relations

  • Related to #324: Stderr possibly not capturing correctly

Closed as Done on 2011-06-24 at 01:21am EDT

@ghost ghost assigned bitprophet Aug 19, 2011

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Jeff Forcier (bitprophet) posted:


This may be related to #324 and should be double-checked after that bug is fixed. If it's still a problem afterwards, we'll need to patch contains so it either looks at succeeded (as in the description) or passes combine_stderr=False, pty=False to its inner run/sudo calls.


on 2011-04-27 at 12:59pm EDT

Member

bitprophet commented Aug 19, 2011

Jeff Forcier (bitprophet) posted:


This may be related to #324 and should be double-checked after that bug is fixed. If it's still a problem afterwards, we'll need to patch contains so it either looks at succeeded (as in the description) or passes combine_stderr=False, pty=False to its inner run/sudo calls.


on 2011-04-27 at 12:59pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Jeff Forcier (bitprophet) posted:


Patch is actually against master/1.1, switching target version due to laziness =)


on 2011-06-24 at 01:14am EDT

Member

bitprophet commented Aug 19, 2011

Jeff Forcier (bitprophet) posted:


Patch is actually against master/1.1, switching target version due to laziness =)


on 2011-06-24 at 01:14am EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Jeff Forcier (bitprophet) posted:


Applied in changeset commit:7f6cbc9d9186270100c0c883a98ae7a37cc52846.


on 2011-06-24 at 01:21am EDT

Member

bitprophet commented Aug 19, 2011

Jeff Forcier (bitprophet) posted:


Applied in changeset commit:7f6cbc9d9186270100c0c883a98ae7a37cc52846.


on 2011-06-24 at 01:21am EDT

@bitprophet bitprophet closed this Aug 19, 2011

ramonvanalteren pushed a commit to ramonvanalteren/fabric that referenced this issue Aug 31, 2011

ramonvanalteren added a commit to ramonvanalteren/fabric that referenced this issue Aug 31, 2011

Merge remote-tracking branch 'upstream/master' into logging
* upstream/master: (123 commits)
  Remove confusing, extraneous note re: example
  Fix main loop to look for Task.run()
  Fix up docs.push
  Update tag list for manually generated docs
  Task decorator must be first
  Enhance docs on Task subclass usage
  Dev version for 1.2
  Dev version for 1.1.1
  Version bump for 1.0.2
  Silly/shitty little sanity-test runner
  Fixes #345, contains() returns boolean, not retval.
  Fix I/O race condition
  Formatting
  Add test re #329
  Add 1.0.2 note to 1.1 release docs
  Note that 1.0.2 will contain 0.9.7
  Fixes #329: reboot() broken for partial host strings
  Dogfooding: use new-style tasks, namespaces in core fabfile
  Re #56, don't allow leaf classic modules to pollute new-style trees
  Use FabricTest to isolate namespace tests
  ...

Conflicts:
	fabric/main.py
	fabric/network.py
	fabric/operations.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment