ensure_packages.rb: Modifed to pass hiera parameters (as hash,array) as first argument

[yadavnikhil]

This change will allow to pass values from hiera backend variables as hash in first argument with additional parameters needed.
Existing implementation did not support this as it considers first argument as array by default. So, if additional parameters were needed from hash, like ex. "providers => rpm" etc, will not work. It will by default pass whole hash value as string to "ensure_resource".

It will merge the hash values for package name (key) with defaults & pass each package name to function_ensure_resource in required format for further processing.

Use:
hiera:

packagelist:
  ksh:
    ensure: latest
  mlocate: {}
  myrpm:
    provider: rpm
    source: "/tmp/myrpm-1.0.0.x86_64.rpm"
    install_options:
      --prefix:
        /users/home
  openssl:
    provider: rpm
    source: "/tmp/openssl-1.0.1e-42.el7.x86_64.rpm"

Call:

ensure_packages($packagelist)

It will support array by default if first argument in not hash.

[hunner]

I believe if you want to accomplish this, you can do it in one line:

create_resources('package', hiera_hash('packagelist'))

Adding resources directly to hiera is very not-recommended though; hiera is for data, and any actionable code should be captured in a manifest such as a "base packages" profile.

[hunner]

Thanks for the contribution, but I think that we prefer to not steer users in this direction. It is still possible to do as I described above, but without over-complicating the exsting ensure_packages() function.

[yadavnikhil]

Hi,

Thanks for considering, but what I want to achieve is one aspect of ensure_packages(); which is multiple declaration of same packages in different modules even with hiera data check.
Hiera is for data only, ensure_packages() will check if any duplicate declaration for a package (same options only) & won't fail my puppet run in such scenarios.
create_resources() will fail in such case with duplicate declaration. Hence asked for this change.

With above change anyway, existing functionality is intact, it just give provision to run with hiera data as well.

Can it be done?

[yapale]

I didnt understand why its rejected ?
its very good idea to support hiera values for duplicate declaration

[hunner]

Ah, I closed it because I didn't understand that the implied purpose of this PR was to allow this functionality without duplicate conflicts. Looking at it again.

[hunner]

So looking at this from an abstract point, if we take the body of changes you are making to ensure_packages() and instead make a new function called ensure_resources(), then ensure_packages() can just call ensure_resources() and if anyone needs this for things OTHER than packages, they will have the ability available then. Let me know what you think!

[yadavnikhil]

This sounds a great idea.
So, as per my understanding, we can keep existing functionality of ensure_packages() & create a new function ensure_resources() as extension to ensure_resource() to achieve this & it will support other resource type as well.
Also, I think modifying ensure_packages() to use this new function will result in duplication, use this new function instead.

Keeping ensure_packages() existing functionality will not break anything for any user using it for now.
I created a new file & reverted ensure_packages() to old one as you see in my forked branch.
You can review and comment.

[hunner]

I think this is a copied comment?

[bmjen]

@yadavnikhil this is a good refactor! I'd also like to see the original scope of the PR included as well, which is refactoring/simplify ensure_packages to simply call ensure_resources.

[hunner]

@bmjen Hmm. That would cause ensure_packages() to be polymorphic again, where arguments are String [Hash] in the original case or Hash in the new case. Do we want that added complexity?

[hunner]

Or maybe I'm entirely wrong about how ensure_packages() works at all.

[yadavnikhil]

@bmjen Yes, you right, we can have ensure_packages() to use ensure_resources(). I was under assumption that it will be duplication as we could directly use ensure_resources() for it. But, now i also think we can extend ensure_packages() functionality as well.
Also, regarding ensure_resources(), you right, we only support Hash in it. For others we can use existing ensure_resource().

@hunner Thanks for pointing out that copied comment, I removed it from latest commit.

I have modified & committed the code suggested.

1. ensure_resources() will only support Hash type argument, it will raise Error for other types.
2. ensure_packages() will call ensure_resources() for Hash type argument, other functionality remains same.

Let me know if this will be fine.
Thanks.

[hunner]

@yadavnikhil I think that looks good. Could you add the documentation to the README and squash it to one commit? Thanks!

[yadavnikhil]

@hunner As suggested, I've modified the README documentation for this new function & updated usage for ensure_packages() in a single commit.
You can modify README if needed.
Thanks.

[hunner]

I think this has to be ensure_resources('user', hiera_hash('userlist'), {'ensure' => 'present'})

[yadavnikhil]

@hunner Yes, correct. Fixed it.

[hunner]

Oops, sorry I didn't catch this before, but this appears to be hard tabs. Could you change all indentation to Ruby's standard 2-space indentation?

[yadavnikhil]

@hunner Fixed the mentioned file as suggested.