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

zypper multipackage patch #4231

Merged
merged 3 commits into from
Jan 26, 2016
Merged

zypper multipackage patch #4231

merged 3 commits into from
Jan 26, 2016

Conversation

lamont-granquist
Copy link
Contributor

replaces #4102

@lamont-granquist
Copy link
Contributor Author

@gsaslis can you look this over?

it uses the new use_multipackage_api declaration, and it cleans up some of the tortured logic in load_current_resource. i think its much more readable than where you were heading.

about the only question i've got (apart from if it even works or not, because i don't have a suse box easily at hand right now) is what that exception in load_current_resource is all about, because it looked like well intentioned but incorrect code:

  • if zypper info was causing that exception to be thrown if both the package was not installed and not available then that was wrong and we simply need to set both to nil and let the superclass deal with the exception in define_resource_requirements
  • if this was trying to catch zypper: command not found kinds of things then maybe we could covert it to a shell_out_with_timeout! with maybe returns: [0, 1] and then if the whole zypper universe is fatally broken on the box then we just let shell_out explode and report the exact failure to the user.

@lamont-granquist
Copy link
Contributor Author

tested this on an opensuse virt and it seems to work fine.

@gsaslis
Copy link
Contributor

gsaslis commented Dec 8, 2015

@lamont-granquist yes - that use_multipackage_api certainly makes things much simpler and much more readable (!!)

👍 looks good to me

Noobie question, (but I am one with Chef, so it's ok) : how would I go about testing this chef branch with my own VM / box ? is there a link / doc somewhere? (I was expecting to find something on https://github.com/chef/chef/blob/master/CONTRIBUTING.md , but it doesn't seem to be mentioned there... )

@lamont-granquist
Copy link
Contributor Author

check out:

http://lists.opscode.com/sympa/arc/chef/2015-10/msg00010.html

probably you want something like:

% gem install appbundle-updater
% sudo appbundle-updater chefdk chef master

or

% gem install appbundle-updater
% sudo appbundle-updater chef chef master

depending on if you're updating chef inside omnibus-chef or omnibus-chefdk...

@lamont-granquist
Copy link
Contributor Author

oh except obviously this isn't merged to 'master' so use 'lcg/zypper-multipackage' instead...

@lamont-granquist
Copy link
Contributor Author

BTW, If you can flame-test this really quickly we might be able to get this into 12.6.0, otherwise it'll get delayed probably until next year...

@lamont-granquist
Copy link
Contributor Author

(looks like this'll slip to january)

@gsaslis
Copy link
Contributor

gsaslis commented Dec 9, 2015

@lamont-granquist: yeah, sorry dude, won't have much time in the week to look at this (looks like you're not the only ones pushing for releases before xmas - and there's a day job as well ;) )

@lamont-granquist
Copy link
Contributor Author

okay, that's cool... we're trying to get a monthly tempo on releases, so hopefully it won't slip too much...

@lamont-granquist lamont-granquist added this to the Accepted Minor milestone Dec 15, 2015
@lamont-granquist
Copy link
Contributor Author

@gsaslis do you have any time to look at this?

@gsaslis
Copy link
Contributor

gsaslis commented Jan 8, 2016

I will this weekend, yes! (if it can wait ; ) )

Yorgos Saslis
Software Engineer

On 8 January 2016 at 01:10, Lamont Granquist notifications@github.com
wrote:

@gsaslis https://github.com/gsaslis do you have any time to look at
this?


Reply to this email directly or view it on GitHub
#4231 (comment).

@lamont-granquist
Copy link
Contributor Author

totally, it looks like i need to rebase it anyway...

@@ -154,14 +145,14 @@ def shell_out_expectation!(command, options=nil)
"zypper --non-interactive --no-gpg-checks install "+
"--auto-agree-with-licenses emacs=1.0"
)
provider.upgrade_package("emacs", "1.0")
provider.upgrade_package(["emacs"], ["1.0"])
end
it "should run zypper upgrade without gpg checks" do
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what
"should run zypper upgrade without gpg checks" really offers over "should run zypper update without gpg checks" (https://github.com/chef/chef/pull/4231/files#diff-b638b95483df994a2982f5a0cfade9d3R132) - perhaps the latter should be removed altogether?

(unless one of the two was meant to test the dist-upgrade, dup Perform a distribution upgrade. feature of zypper)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on tests with the Chef::Config variable set to false, the other tests the default.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok I see, then perhaps just changing the test's title would help?

@gsaslis
Copy link
Contributor

gsaslis commented Jan 11, 2016

@lamont-granquist with regards to your question about that exception being thrown:
zypper info can return an error code in a few cases where it cannot complete successfully, but not when it cannot find the package (in that case, the return code is 0, unless you pass in some weird input, such as $1 as the package name ) .
I think that probably leads us on to your 2nd bullet, and if you really just needed a command you could run, so you could be sure that "zypper is all set up nicely" and that your issue is irrelevant with zypper info and packages, etc. then according to the zypper manual you can use zypper ping that does just that and returns 0 ; )

Hope that helps!

@lamont-granquist
Copy link
Contributor Author

Yeah, I think I answered those bullet points for myself. I just changed it to shell_out_with_timeout! so that the exception (which really truly is exceptional in this case and indicates zypper has gone insane) will just blow up and it'll get reported to the user. An exception is the correct behavior, but there was nothing useful gained from decorating the exception from shell out with a Chef::Exceptions::Package error.

@lamont-granquist
Copy link
Contributor Author

@chef/client-core i think this is ready for review

@lamont-granquist
Copy link
Contributor Author

@chef/client-core still needing review...

@thommay
Copy link
Contributor

thommay commented Jan 21, 2016

👍 merge once rebased

lamont-granquist added a commit that referenced this pull request Jan 26, 2016
@lamont-granquist lamont-granquist merged commit f6da39a into master Jan 26, 2016
@lamont-granquist lamont-granquist deleted the lcg/zypper-multipackage branch January 26, 2016 01:23
@mwrock mwrock added Bug and removed Bug labels Mar 2, 2016
@thommay thommay added Type: Enhancement Adds new functionality. and removed Enhancement labels Jan 25, 2017
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Enhancement Adds new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants