(#9293) Yumrepos should be ensurable

[dLobatog]

This change adds a provider to yumrepo that makes it ensurable.
So far we needed todo a workaround making each of the individual files
that yumrepo creates ensurable, now yumrepo itself does it.

This probably should belong in 3.x as well as in 2.7x

https://projects.puppetlabs.com/issues/9293

[zaphod42]

This should only go onto master, not onto 2.7.x since it is a new feature.

It seems like if a provider is going to be created for yumrepo, there should be some refactoring to move the provider portions of the type into the provider, otherwise we'll end up with another convoluted type/provider arrangement.

[domcleal]

Luke did some work to split the yumrepo type and provider in #8758, but apparently the patch isn't in a working state today.

[dLobatog]

Take a look at the newest changes. In short, it checks reposdir in yum.conf, it looks for a repo in other files aside from its usual location. (i.e repo "epel" might not be in reposdir/epel.repo). It doesn't create a new yumrepo if the properties are different but the repo is there, as stschulte said ("exists? should return true even though enable, baseurl or other properties might not be correct") but honestly this sounds broken to me, if I update my puppet manifests with new information on my yumrepos, wouldn't I always expect them to be updated, regardless if they were there before or not?

Is there any easy way we can move the PR to master or should I do it again?

dLobatog@d07eb39

[jeffmccune]

Thank you for contributing to Puppet!

Is there any easy way we can move the PR to master or should I do it again?

There isn't unfortunately. A new pull request needs to be submitted, but I notice this pull request is already referring to puppetlabs:master, so you should be good to go already.

If you'd like to squash the two commits together, then git rebase -i is probably the best tool for the job.

I'll leave the remaining questions for the other people participating on this pull request; @stschulte and @domcleal have this well in hand.

The only comment i have is my boilerplate textexpander comment about tests:

Code - missing examples

Before we merge this into Puppet examples that exercise this change in behavior need to be included in the patch. If you're not comfortable adding RSpec examples, I'm happy to help out. I'm jmccune in #puppet-dev on freenode. Please include examples that will automatically let us know if this problem resurfaces in the future, otherwise we may have a regression as we continue to make changes over time.

[dLobatog]

Oh I'm happy about it, in fact I was even feeling bad about not including any specs, I just didn't include any as it looked like I didn't fully grasp the behavior you guys intended to get with this feature! Thanks for the reminder

[dLobatog]

Quick question for a sample case:

If we have a repository that has already been defined somewhere on reposdir, but we have a new attribute, for instance baseurl we want to modify, should "exists?" really return true just because the repository is in there?
If it returns true, that means we're not updating the yumrepo config, which is what I wanted puppet to do.

If ensurable is really meant to not overwrite the repo even if the configuration is not up-to-date, how should I do this? https://projects.puppetlabs.com/issues/9293

[stschulte]

If we have a repository that has already been defined somewhere on reposdir, but we have a new attribute, for instance baseurl we want to modify, should "exists?" really return true just because the repository is in there?

That is exactly true because as you said - the repository does exist. The method "baseurl" would have to return :absent in your case so puppet would be like "is the ensure property in sync? Let's check exists?. Hm exists? returns true and the user specified ensure => present so the ensure property is in sync. Go to the next property: Is the baseurl property in sync? Let's run the method baseurl. It does return :absent but the user specified baseurl => http://foo.bar/baz so baseurl is out of sync. Let's run baseurl= and pass http://foo.bar/baz as an argument to fix that.

You actually have to define a getter and a setter method for each resourceproperty

[dLobatog]

I think mk_resource_methods creates getters and setters automatically. Anyway I noticed that puppet automatically writes back the missing properties using transaction/resource_harness.rb (lines around 40). Is there anything missing other than the specs then?

Lines 8 to 30 on type/yumrepo.rb handle when a property is in sync or not, I so if the repository is exists (ensure in sync) and baseurl returns :absent but the user specified something else (baseurl out of sync), it goes ahead and change it.

I am clearly missing something if this is a job the provider should do

[stschulte]

mk_resource_methods creates getters and setters to read/write the instance variable @property_hash. That will naturally only be useful if your provider first populates @property_hash with the actual values. Note that the provider always represents the acutal state of a resource, so @property_hash[:baseurl] has to be the current baseurl and not the desired baseurl (the desired value can be queried with resource[:baseurl]).

Your reference to resource_harness is perform_changes I guess? Essentially this is where the base functionality is:

The method first calls resource.retrieve_resource which will call retrieve on all properties (baseurl, enable, gpgkey etc) to get the current values. This will normally delegate to the provider's get methods (e.g. baseurl). perform_changes then collects the desired values (the ones the user has specified). It will then compare the current values with the actual values. If the value is not in sync (let's say the baseurl is incorrect) we call sync on the (baseurl)property. This will in turn deligate to the provider's set method (baseurl=(new fancy value) in this case). As a result your provider "only" has be able to get the current state and be able to store an updated state. It does not have to check insync itself.

Note if your set methods only update the content of @property_hash (that is what mk_resource_methods will do for you) your provider will never write the actual content back to disc. To actually do that you can implement a flush method. Puppet will call a flush provider method after it has synced all properties and if at least one property was out of sync.

To the yumrepo type: It is not strictly necessary to have a provider at all and the yumrepo type is an example for that. In this case the type has to do all the work of retrieving the actual content. If we now want to migrate to a type <-> provider structure, the type will probably look like this:

module Puppet
  # Doc string for properties that can be made 'absent'
  ABSENT_DOC="Set this to `absent` to remove it from the file completely."

  newtype(:yumrepo) do
    @doc = "The client-side description of a yum repository. Repository
      configurations are found by parsing `/etc/yum.conf` and
      the files indicated by the `reposdir` option in that file
      (see `yum.conf(5)` for details).

      Most parameters are identical to the ones documented
      in the `yum.conf(5)` man page.

      Continuation lines that yum supports (for the `baseurl`, for example)
      are not supported. This type does not attempt to read or verify the
      exinstence of files listed in the `include` attribute."

    newparam(:name) do
      desc "The name of the repository.  This corresponds to the
        `repositoryid` parameter in `yum.conf(5)`."
      isnamevar
    end

    newproperty(:descr) do
      desc "A human-readable description of the repository.
        This corresponds to the name parameter in `yum.conf(5)`.
        #{ABSENT_DOC}"
      newvalue :absent
      newvalue /.*/
    end

    newproperty(:mirrorlist) do
      desc "The URL that holds the list of mirrors for this repository.
        #{ABSENT_DOC}"
      newvalue :absent
      newvalue /.*/)
    end

    newproperty(:baseurl) do
      desc "The URL for this repository. #{ABSENT_DOC}"
      newvalue :absent
      newvalue(/.*/)
    end
    [...]
  end
end

So the type will just define the properties of a yumrepo and all the rest should be handled by the provider. Does this make sense?

If you have further questions you can also join the google group puppet-dev. This way you will probably reach a wider audience than commenting on a single pull request.

[dLobatog]

"So the type will just define the properties of a yumrepo and all the rest should be handled by the provider. Does this make sense?"

Yeah, probably this is the bit I was missing. Since my type was already handling fixing missing/broken properties, I thought I wouldn't be necessary to define the flush method. I've noticed there are a few methods on the Puppet type, self.store, flush, section, etc.. that do the job I want to achieve on the provider, I guess moving them over the provider will suffice? What about the insync?/sync/retrieve/inikey methods who don't specifically belong in the type?

I'm trying to contact you or anyone else who knows about this over #puppet-dev (IRC) with no luck, will ask on the google puppet-dev group then!

[stschulte]

Hi @elobato,

the instances method etc should go in the provider. Wether you can just move them unmodified really depends on how far you want to refactor.

The current yumrepo type defines its own subclass for all its properties. If you just say newparam :foo you'll get a property of the class Puppet::Property but the yumrepo properties are dervied from Puppet::IniProperty. I would drop these subclass because the type should not make the assumption that the backend storage are ini files. In the future yum repositories may be created with a command line tool and then it would be impossible to write a second provider that uses the CLI. But another one might say that as long as we do only have one provider (which handles ini files) the current propertie classes are ok.

[jeffmccune]

I'm taking a look at reviewing this now. As some initial checks, please watch out for whitespace errors which can be checked for using git diff --check.

Housekeeping - Whitespace Errors

(1.9.3-p327 bundler)[jeff@mccune] (eLobato-fix/master/ensurable_yumrepo)(clean) /workspace/puppet-3.x/src/puppet
$ git diff --check master
lib/puppet/provider/yumrepo/yumrepo.rb:21: trailing whitespace.
+    file = Puppet::Util::IniConfig::File.new 
lib/puppet/provider/yumrepo/yumrepo.rb:38: trailing whitespace.
+    file = Puppet::Util::IniConfig::File.new 
lib/puppet/provider/yumrepo/yumrepo.rb:44: trailing whitespace.
+    
lib/puppet/provider/yumrepo/yumrepo.rb:92: trailing whitespace.
+end 
lib/puppet/type/yumrepo.rb:210: trailing whitespace.
+      end 

Also, please keep our commit message guidelines in mind. Here's my canned comment on the subject. I mention this specifically because both commit messages aren't imperative, for example we prefer "(#9293) Make yumrepo resources ensurable" and not "(#9293) Yumrepos should be ensurable". The body of this commit message is OK, but it lacks an explanation of why it's important for you to make the yumrepo ensurable. Why is it that you need to work around the issue today? The commit message in bbb2768 could also be improved. "Modified provider so it doesnt use external dependencies. Added support for reposdir and fixed bugs" doesn't tell me the three pieces of information I really need to understand the change. What is the behavior of Puppet without bbb2768 applied? Why is this a problem? How does bbb2768 change the behavior of Puppet to address the problem?

I'll likely have more comments about the code itself as well and will follow up soon with those. The housekeeping issues are definitely next steps, however, and need to be addressed before we'll merge this pull request in.

Housekeeping - Commit Message

On the point of housekeeping, please amend the commit message to include a description of the behavior without the patch applied, why this behavior is a problem, and then describe how the change addresses the problem. If the change addresses an error, please consider including the error message verbatim so it's clear if the problem resurfaces in the future. The first line of the commit message should be an imperative statement in the present tense. Change rather than changed or changing for example.

[jeffmccune]

This shouldn't be here. Perhaps it was overlooked when committing after doing some debugging?

[jeffmccune]

I've taken a look at the code itself, and things look pretty good overall. As next actions, the existing examples need to pass (please see the comment regarding the examples this change causes to fail), new examples need to be included to exercise the new ensureable behavior, and some housekeeping needs to be performed on the commit message and code style.

I'm going to go ahead and close this pull request for the time being. Please re-open this pull request once the next actions are addressed, new information is available, or you have a question related to this pull request. Closing the pull request doesn't mean we don't consider this change valuable, just that there are things that need to be addressed before it can be merged. If you have any questions or concerns, please don't hesitate to ping us in #puppet-dev on irc.freenode.net.

[mcanevet]

@elobato could you please finish this patch ? I'd really like to see it merged.

[Sharpie]

@mcanevet Another attempt at implementing this feature is underway in #2086.