Small fixes to RPM module

[movitto]

Not needed for bundler_ext anymore, but still should be resolved. Handles a few edge cases with different versions of rpm on different platforms

[coveralls]

Coverage remained the same when pulling a7f5296 on movitto:rpm-fix into 385ee8c on ManageIQ:master.

[Fryguy]

Can this be handled through the Distro::COMMANDS?

[bdunne]

Since we have the Distro's, I would suggest using them. Also, Fedora and RHEL should be listed as different distro's.

[coveralls]

Coverage remained the same when pulling a70cff5 on movitto:rpm-fix into 385ee8c on ManageIQ:master.

[movitto]

@Fryguy @brandondunne updated. Going forward it might not be as simple as just splitting this by distro as the LSB standard incorporation may vary between releases of a distro, eg the move to /usr wasn't too long ago [1], and assumably a future version of RHEL will incorporate this change. That being said we can tackle that issue when we get there

[1] https://fedoraproject.org/wiki/Features/UsrMove

[Fryguy]

Looks good to me. @jrafanie @brandondunne ?

[bdunne]

I'm not a fan of chaining on multiple lines... so I would look to do something like this to improve readability:

arguments = [described_class.rpm_cmd, :params => {"-qi" => "ruby"}]
result = CommandResult.new("", data, "", 0)

described_class.should_receive(:run).with(arguments).and_return(result)

[bdunne]

@movitto Overall a great change. I agree with your concerns about the path of the commands, especially with RHEL 7 coming up.

[jrafanie]

I'm not a big fan of more than one elsif.

The "detect_distro_from_etc_issue" and "detect_distro_from_etc_release" logic can be moved to methods in a future pull request to simplify the logic here.

[coveralls]

Coverage remained the same when pulling 5e129fe on movitto:rpm-fix into 385ee8c on ManageIQ:master.

[movitto]

@Fryguy @brandondunne @jrafanie updated

[jrafanie]

@movitto I like this better but I think the specific Distro should be responsible for detecting itself.

Maybe something like this? We can do this in a follow-up pull request if desired.

module Distros
  class Distro
    ETC_ISSUE_KEYWORDS = []
    RELEASE_FILE = ''
    LIST_OF_DETECTABLE_DISTROS = %w(Ubuntu Fedora RHEL)  # this be done programmatically

    def self.etc_issue
      @etc_issue ||=  File.read('/etc/issue') unless File.exists?('/etc/issue')
    end

    def self.etc_issue_keywords
      self::ETC_ISSUE_KEYWORDS
    end

    def self.release_file
      self::RELEASE_FILE
    end

    def self.local
      # this can be cleaned up..
      @local ||= begin
        result = nil
        LIST_OF_DETECTABLE_DISTROS.each do |distro|
          distro = Distros.const_get(distro)
          result = distro.new if distro.detected?
        end
        result ||=  Distros.generic
        result
      end
    end

    def self.detected?
      detected_by_etc_issue? || detected_by_etc_release?
    end

    def self.detected_by_etc_issue?
      etc_issue_keywords.any? {|k| etc_issue.to_s.include?(k)}
    end

    def self.detected_by_etc_release?
      File.exists?(release_file)
    end
  end

  class RHEL < Distro
    RELEASE_FILE =       "/etc/redhat-release"
    ETC_ISSUE_KEYWORDS = ['red hat', 'Red Hat', 'centos']
  end

  class Ubuntu < Distro
    # TODO
  end

  class Fedora < Distro
    # TODO
  end
end

[Fryguy]

@movitto Status on this? @jrafanie Had a suggestion on organization...thoughts?

[movitto]

@Fryguy wasn't sure if the suggestion was to be incorporated into another PR or this one. In any case will update to include the changes as suggested

[coveralls]

Coverage remained the same when pulling 65cd646 on movitto:rpm-fix into a93bcab on ManageIQ:master.

[movitto]

@Fryguy @jrafanie updated

With the suggested patch @etc_issue would've been read / created for each distro instance. So changed this to split out EtcIssue into its own utility class similar to fstab

[Fryguy]

Prefer .to_sym over .intern for consistency.

[jrafanie]

It's looking good @movitto. I had a few comments above.

[movitto]

@Fryguy @jrafanie updated

Incorporated feedback, added Distros.local helper method, reset @Local via global hook, etc

Perhaps at some point in the future we can add custom rspec matchers / helpers for use here and in other projects that stub out some common functionality at the linux system admin level. Not sure, haven't played around w/ that feature too much myself.

[jrafanie]

Looks better @movitto 👍

@Fryguy @brandondunne I'm good with this. I'm not sure if there are other pulls that should get in before this.

[jrafanie]

@movitto This can't be merged, please rebase and push.

@brandondunne Are there other bug fixes we need to get in before we merge this structural change?

[movitto]

@jrafanie rebased

[jrafanie]

Shouldn't this be detect or find?
We should return the first class that is detected?, right? Or return the generic one.

[jrafanie]

@movitto Like you said, some of this needs to be cleaned up. But we were able to run it against rhel and test the Rpm.info and Rpm.list_installed and they worked.

The one comment above should be fixed, but I think we can merge after that.

[cfme-bot]

Checked commits movitto@f8ce567 .. movitto@37c040b with rubocop
15 files checked, 5 offenses detected

This comment is on an outdated set of commits.

[jrafanie]

@movitto I think this is ready to merge once the rubocop changes are made or waived.

[cfme-bot]

This pull request is not mergeable. Please rebase and repush.

[cfme-bot]

Checked commit movitto@57c323a with rubocop
15 files checked, 0 offenses detected

Everything looks good. 👍

[movitto]

@jrafanie updated

[coveralls]

Coverage remained the same when pulling 57c323a on movitto:rpm-fix into 73dbb7c on ManageIQ:master.