several questions regarding pkg.install enhancements

[Erik Johnson]

I have three questions for Tom, et al, regarding the features I've been working on lately. Please let me know your opinions on each of the below:


I'm currently working on allowing pkg.install to accept multiple packages in a single call on various package providers, a feature only presently available in the yumpkg provider. Additionally, I am adding where it doesn't exist the ability to install a package from a file. The later feature uses a new keyword argument, "sources".

Currently, since not all package providers have both of these features implemented, the pkg state does not yet support either of these options. However, thinking forward to how this would be implemented in states, it seems natural to accept "sources" as a list. However, you cannot submit the sources as a list via CLI, you need to use a comma separated string, which is then split into a list.

My first question is this: would it be fine to accept both the package names and sources as either a string or a list? Some try/except logic could be used within the different package providers' respective install() functions to split if needed.

If it is implemented in this way, then both of the following would do the same thing:

method_1:
  pkg.installed:
    - names:
      - foo
      - bar
      - baz
    - sources:
      - salt://foo.rpm
      - http://somesite.org/bar.rpm
      - ftp://somesite.net/baz.rpm

method_2:
  pkg.installed:
    - name: foo,bar,baz
    - sources: salt://foo.rpm,http://somesite.org/bar.rpm,ftp://somesite.net/baz.rpm


I'm not sure if this be considered "OK", or if the ambiguity would not be desired. If not, then the only option would be only to accept the input as a string. In other words, method_1 above would not work, and method_2 would be the only valid way of submitting multiple package sources. Personally, I like the idea of flexibility, and method_1 above would look great in SLS, but I'd like to get some feedback before I write the code and submit a pull req.


Second question: In yumpkg.py, which has for some time had the ability to install multiple packages from the repositories in a single call, the code currently allows for either submitting the packages and sources as a comma-separated or space-separated list. I propose removing the option to use a space as the delimiter, and only allowing comma-separated lists. This is an area where I think ambiguity is not a good thing. Thoughts?


Third question: Providers like yum and zypper both allow you to mix RPM and repository packages. That said, it follows that you should be able to mix RPM and repo sources in the same ID declaration. Here's my proposal for doing this, does it look good?

method_1:
  pkg.installed:
    - names:
      - foo
      - bar
      - baz
    - sources:
      - salt://foo.rpm
      - None
      - ftp://somesite.net/baz.rpm

method_2:
  pkg.installed:
    - name: foo,bar,baz
    - sources: salt://foo.rpm,None,ftp://somesite.net/baz.rpm

In this case, a value of None (or 'None', if submitted as a string via method_2 above, or via CLI) would tell the install() function to use the repository instead of an RPM/deb/etc. package file. It may seem a little confusing to see None as a source, but the docstring for the individual pkg.install() functions would clear that up.


Thanks in advance for your input,

--

-Erik

"For me, it is far better to grasp the universe as it really is than to persist in delusion, however satisfying and reassuring."  -- Carl Sagan

[Thomas S Hatch]

The big thing here is names, you need to remember that names splits up the state calls, it cannot be used here, like this. Next is the fact that mixing repo packages and download packages will be non-sensical, the underlying function should be atomic, and having an install function call yum install or rpm -Uvh (or pacman for instance) more than once would be a bad move. I propose something like this:

rfc_pkg:

  pkg.installed:

    - sources:

      - foo: salt://foo.rpm

      - bar: ftp://somesite.net/bar.rpm

      - baz: http://othersite.com/baz.rpm

And then for multiple repo packages

repo_pkgs:

  pkg.installed:

    - pkgs:

      - foo

      - bar

      - baz

This means that pkg.installed should throw an error if pkgs and sources are defined, also it allows for groups to be installed in an atomic way, and since we use dedicated kwargs (sources and pkgs) this functionality can be easily limited to specific providers, since if you pass a kwarg down that is not used then the underlying function will error out.

What do you think?

[Thomas S Hatch]

Also, thanks for asking, keeping the data structures clean is important, and we need to do it so that people don't make crazy inputs for sls files

[Erik Johnson]

Ahh yes, I did not consider the fact that "names" would create multiple calls to the install function. I like the example you proposed.

So, given your example, I'm making the following assumptions. Let me know if we're both on the same page.

1) Both "pkgs" and "sources" would be keyword arguments
2) If "pkgs" is specified, the multiple-package logic would be invoked for repo packages
3) If "sources" is specified, the multiple-package logic would be invoked for package files (rpm/deb/etc.)
4) If either is submitted, then "name" is ignored entirely.

Given that the above are all true, what would be done if both pkgs and sources are specified? My guess would be that the state would nip that in the bud before even calling pkg.install, exiting with a False outcome and a message describing that you can't use both. For the module function, logging at loglevel "error" and returning an empty dict seems to be the way to go.


So, how would youenvision the CLI invocation of pkg.install working, given that for "sources" you have a list of dicts being passed to the state? I can just pass that list of dicts to pkg.install when I call it from the state, but if I'm running the pkg.install function from the CLI then I will need an agreed-upon format that I can transform into the same list of dicts that the state would pass to the function.

Also, if assumption #4 above holds true, then "name" is ignored. However, "name" is a positional argument, so when running via CLI you would have to put something there, a value that would be ignored anyway. Sounds a bit clunky. A workaround to this would be to make "name" a keyword argument, but I believe (and correct me if I am wrong) that doing so would require you to change the positional argument to a keyword argument in every other module function that gets invoked by a state, as well as in all the states.

[Thomas S Hatch]

On Wed, Nov 14, 2012 at 10:54 PM, Erik Johnson <pale...@gmail.com> wrote:

Ahh yes, I did not consider the fact that "names" would create multiple calls to the install function. I like the example you proposed.

So, given your example, I'm making the following assumptions. Let me know if we're both on the same page.

1) Both "pkgs" and "sources" would be keyword arguments
2) If "pkgs" is specified, the multiple-package logic would be invoked for repo packages
3) If "sources" is specified, the multiple-package logic would be invoked for package files (rpm/deb/etc.)
4) If either is submitted, then "name" is ignored entirely.

You got it

 

Given that the above are all true, what would be done if both pkgs and sources are specified? My guess would be that the state would nip that in the bud before even calling pkg.install, exiting with a False outcome and a message describing that you can't use both. For the module function, logging at loglevel "error" and returning an empty dict seems to be the way to go.

Right again

 

So, how would you envision the CLI invocation of pkg.install working, given that for "sources" you have a list of dicts being passed to the state? I can just pass that list of dicts to pkg.install when I call it from the state, but if I'm running the pkg.install function from the CLI then I will need an agreed-upon format that I can transform into the same list of dicts that the state would pass to the function.

Use a dict from the command line, Salt supports that:

salt \* pkg.install 'sources={"foo": "salt://foo.rpm"}'

 

Also, if assumption #4 above holds true, then "name" is ignored. However, "name" is a positional argument, so when running via CLI you would have to put something there, a value that would be ignored anyway. Sounds a bit clunky. A workaround to this would be to make "name" a keyword argument, but I believe (and correct me if I am wrong) that doing so would require you to change the positional argument to a keyword argument in every other module function that gets invoked by a state, as well as in all the states.

Good thinking, this would be a problem. But yes, as we add in support for more platforms we should make the name argument default to None, Salt will handle the argument mapping is name is an arg or kwarg

[Erik Johnson]

On Thu, Nov 15, 2012 at 11:04 AM, Thomas S Hatch <that...@gmail.com> wrote:

On Wed, Nov 14, 2012 at 10:54 PM, Erik Johnson <pale...@gmail.com> wrote:

Ahh yes, I did not consider the fact that "names" would create multiple calls to the install function. I like the example you proposed.

So, given your example, I'm making the following assumptions. Let me know if we're both on the same page.

1) Both "pkgs" and "sources" would be keyword arguments
2) If "pkgs" is specified, the multiple-package logic would be invoked for repo packages
3) If "sources" is specified, the multiple-package logic would be invoked for package files (rpm/deb/etc.)
4) If either is submitted, then "name" is ignored entirely.

You got it

 

Given that the above are all true, what would be done if both pkgs and sources are specified? My guess would be that the state would nip that in the bud before even calling pkg.install, exiting with a False outcome and a message describing that you can't use both. For the module function, logging at loglevel "error" and returning an empty dict seems to be the way to go.

Right again

 

So, how would you envision the CLI invocation of pkg.install working, given that for "sources" you have a list of dicts being passed to the state? I can just pass that list of dicts to pkg.install when I call it from the state, but if I'm running the pkg.install function from the CLI then I will need an agreed-upon format that I can transform into the same list of dicts that the state would pass to the function.

Use a dict from the command line, Salt supports that:

salt \* pkg.install 'sources={"foo": "salt://foo.rpm"}'

 

Excellent, I was not aware of this. So, to pass two sources, I assume the below example would be what is needed, am I right?

salt \* pkg.install 'sources=[{"foo": "salt://foo.rpm"},{"bar": "salt://bar.rpm"}]'

 

Also, if assumption #4 above holds true, then "name" is ignored. However, "name" is a positional argument, so when running via CLI you would have to put something there, a value that would be ignored anyway. Sounds a bit clunky. A workaround to this would be to make "name" a keyword argument, but I believe (and correct me if I am wrong) that doing so would require you to change the positional argument to a keyword argument in every other module function that gets invoked by a state, as well as in all the states.

Good thinking, this would be a problem. But yes, as we add in support for more platforms we should make the name argument default to None, Salt will handle the argument mapping is name is an arg or kwarg

 

OK. Looking back, I realize that I was confusing an argument that has a default value with an actual keyword argument (**kwargs). Since pretty much any module function that a state would call (maybe all, haven't checked) contains "name" followed by arguments which all have default values, the state functions themselves would not need to be modified.

So, for the package providers that I am modifying, it should be no problem for me to assign "name" a default value of None, correct?

[Thomas S Hatch]

Correct on all counts!

[Erik Johnson]

On Thu, Nov 15, 2012 at 2:19 PM, Thomas S Hatch <that...@gmail.com> wrote:

Correct on all counts!

Fantastic. Now that that's settled, I can write some code!

[Thomas S Hatch]

Awesome, lets target 0.10.6, since we are trying to get 0.10.5 out the door today or tomorrow!

[Erik Johnson]

On Thu, Nov 15, 2012 at 2:24 PM, Thomas S Hatch <that...@gmail.com> wrote:

Awesome, lets target 0.10.6, since we are trying to get 0.10.5 out the door today or tomorrow!

Sounds doable. Hope you're OK with some of the package providers having the ability to install from package files and some not, I just wasn't able to get to all of them. By 0.10.6 I think this will be mostly done.

/me crosses fingers
 

[Thomas S Hatch]

Yep, Salt is designed to handle these differences, thanks for taking this on Erik!

[Erik Johnson]

On Thu, Nov 15, 2012 at 2:44 PM, Thomas S Hatch <that...@gmail.com> wrote:

Yep, Salt is designed to handle these differences, thanks for taking this on Erik!

No prob. This will be fun, and will help our installs run quicker down the road.

Quick couple questions on accepting the "pkgs" and "sources" params as literal python data structures. I'm assuming that I'll need to eval the string into a list should the variable not already be an list instance, correct?

Also, for "sources", rather than scour the source code for examples, can you tell me if there is there a piece of code somewhere in salt that elegantly takes a data structure in the format of a list of 1-element dicts, like [{"foo": "salt://foo.rpm"},{"bar": "salt://bar.rpm"}], and turns it into a single dict, or something else over which I can iterate? I'm thinking of a couple different ways I can accomplish this, but if it's already being done better elsewhere in the project I don't want to re-invent the wheel. I'm looking in salt/state.py, and I see the state_args() function, but this only seems to be returning the names of the arguments passed to a state.


Thanks,

[Erik Johnson]

On Thu, Nov 15, 2012 at 10:27 PM, Erik Johnson <pale...@gmail.com> wrote:

On Thu, Nov 15, 2012 at 2:44 PM, Thomas S Hatch <that...@gmail.com> wrote:

Yep, Salt is designed to handle these differences, thanks for taking this on Erik!

No prob. This will be fun, and will help our installs run quicker down the road.

Quick couple questions on accepting the "pkgs" and "sources" params as literal python data structures. I'm assuming that I'll need to eval the string into a list should the variable not already be an list instance, correct?

Also, for "sources", rather than scour the source code for examples, can you tell me if there is there a piece of code somewhere in salt that elegantly takes a data structure in the format of a list of 1-element dicts, like [{"foo": "salt://foo.rpm"},{"bar": "salt://bar.rpm"}], and turns it into a single dict, or something else over which I can iterate? I'm thinking of a couple different ways I can accomplish this, but if it's already being done better elsewhere in the project I don't want to re-invent the wheel. I'm looking in salt/state.py, and I see the state_args() function, but this only seems to be returning the names of the arguments passed to a state.

The best I have come up with so far is:

>>> all_sources = {}
>>> for d in [{'foo': 1},{'bar': 2}]:
...     all_sources.update(d)
...
>>> all_sources
{'foo': 1, 'bar': 2}
>>>

[Erik Johnson]

The major flaw I see with the above is that it allows the dicts to be more than a single element each. I guess checking len(d) would fix that. Anyway, let me know if you have a better way.

[Thomas S Hatch]

We have a lot of places where we use single element dicts, for instance the arguments in sls files. The solution is to check that they have a length of 1 and reject them otherwise.

Also on evaling, Salt should be doing that on the minion with the publication data.

[Erik Johnson]

On Thu, Nov 15, 2012 at 11:18 PM, Thomas S Hatch <that...@gmail.com> wrote:

We have a lot of places where we use single element dicts, for instance the arguments in sls files. The solution is to check that they have a length of 1 and reject them otherwise.

Also on evaling, Salt should be doing that on the minion with the publication data.

OK. Well, here's what I have thrown together: http://pastebin.com/gbHUTzce

It hasn't been tested so it might very well have syntax or other errors in it, but that's what I've come up with.

[Thomas S Hatch]

Yep, looks like you are on the right track!