Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(#15686) ensure that the error string comparison is done on a string. #985

Closed
wants to merge 2 commits into from
Closed

(#15686) ensure that the error string comparison is done on a string. #985

wants to merge 2 commits into from

Conversation

vrthra
Copy link
Contributor

@vrthra vrthra commented Jul 30, 2012

cleanedup the solaris 10 package provider and added tests.

@@ -0,0 +1,87 @@
#! /usr/bin/env ruby -S rspec
require 'spec_helper'
s=<<EOF
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like something that was left in by accident. I don't think it should be here.

@zaphod42
Copy link
Contributor

This is a lot of change in a single commit and the commit message doesn't have any clear baring on what the change is. Also, the reference bug doesn't help me too much in understanding. After reading the bug I was able to see why one change was made (not using the exception as a string in the error message), but I still don't understand what the rest is trying to achieve.

I think the commit message needs to be expanded and maybe this needs to be split into several chunks.

@vrthra
Copy link
Contributor Author

vrthra commented Jul 30, 2012

thanks, will do that.

@zaphod42
Copy link
Contributor

zaphod42 commented Aug 2, 2012

These tests don't pass on Mac. There doesn't seem to be enough mocking of the underlying system taking place and so I suspect they work on Solaris, but that is because it can execute the commands.

There are also a couple of whitespace errors that should get cleaned up.

@vrthra
Copy link
Contributor Author

vrthra commented Aug 2, 2012

Updated the test cases to fully mock the commands.

end

# Retrieve the version from the current package file.
def latest
hash = info2hash(@resource[:source])
hash[:ensure]
info2hash(@resource[:source])[:ensure]
Copy link
Contributor

Choose a reason for hiding this comment

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

@vrthra we should add the has_feature :upgradeable to the sun provider, so that when ensure => latest, the package type will invoke the provider's latest and update methods to upgrade the package. Or take those methods out if they don't actually work.

- refactor the namemap completely so that it is defined only once
  * We also remove the nil lines so that only keys of interest are retained.
- replace the execpipe with execute since we do not want to exit only halfway through
- refactor the info2hash to parse_pkg and replace the instances and info2hash to use it instead of
  duplicating the code
- cleanup the exception handling to ensure that an exception is used only when it really is an
  exception.
- IMP: This fixes the bug
 * changing from execpipe to execute removes the problem that we were facing, that is,
   execpipe block used to return an array as output which used to cause the exception
   that was raised to have an array as the error message.
 * We no longer rely on exception handling to know when a package is not present. Instead
   we parse the ouput and look for 'ERROR' string.
This ensures that the logic of deciding what extra options to pass are in one place.
- remove redundant parenthesis, In ruby we can avoid the extra syntactic noice of
parenthesis in method calls with no arguments
cmd = [command(:pkginfo), '-l']
cmd << '-d' << device if device
cmd << @resource[:name]
pkgs = self.class.parse_pkginfo(execute(cmd, :failonfail => false))
Copy link
Contributor

Choose a reason for hiding this comment

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

@vrthra did you intentionally not combine the stdout and stderr streams here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@vrthra nevermind about that

@joshcooper
Copy link
Contributor

This was merged in 2df01b0

@joshcooper joshcooper closed this Aug 10, 2012
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.

None yet

3 participants