Added not_if condition to remote_file pull

[donwlewis]

I wanted to be able to pull the certificate RPM package from our satellite instance via HTTPS rather than HTTP. I noticed that there was a method called katello_cert_rpm_installed? which was checking to see if the RPM was already installed. I added it as a not_if condition for the remote_file pull, so that if the certificate was already installed, it would not attempt to pull the certificate file again. This allows for me to add the pull in my on recipe in a separate cookbook.

[adamleff]

@donwlewis thanks for your contribution!

My main concern with this is the use case of a user unregistering from one satellite server and registering to another one. With the way the cookbook is currently written, the new cert will be installed regardless overwriting the previous one, but your proposed logic would prevent that. We don't remove the cert package when unregistering today because the cert package name is not what we fetched (it usually includes the IP address of the satellite host, for example), and we did not add logic to find that RPM and remove it. It could be done though. Just something to consider...

A few things before we'll be able to merge this:

• please back out the metadata version bump. It's likely we'll merge in other changes before we release and we'll bump the version before we do so.
• we'll need to add a test to catch this change.
  • This context block will likely need to be renamed "when the host is not registered and the cert is not installed" ensuring the cert is fetched
  • A new context block will need to be added for "when the host is not registered and the cert is installed" ensuring that the cert is not fetched (the new behavior you are adding)

You'll likely need to add an allow().to receive(:katello_cert_rpm_installed?).and_return() statements to existing context blocks too now that it will be called and depended on.

Please give that a shot and let me know if you have any questions on the tests.

~Adam

[donwlewis]

OK Adam, I will give it a shot. I am new to all of this so I will try to do the best I can. Thanks so much for the help!

On Fri, Jan 29, 2016 at 1:13 PM Adam Leff notifications@github.com wrote:

@donwlewis https://github.com/donwlewis thanks for your contribution!

My main concern with this is the use case of a user unregistering from one satellite server and registering to another one. With the way the cookbook is currently written, the new cert will be installed regardless overwriting the previous one, but your proposed logic would prevent that. We don't remove the cert package when unregistering today because the cert package name is not what we fetched (it usually includes the IP address of the satellite host, for example), and we did not add logic to find that RPM and remove it. It could be done though. Just something to consider...

A few things before we'll be able to merge this:

• please back out the metadata version bump. It's likely we'll merge in other changes before we release and we'll bump the version before we do so.
• we'll need to add a test to catch this change.
  • This context block https://github.com/chef-partners/redhat-subscription-manager-cookbook/blob/master/spec/unit/resources_spec.rb#L58 will likely need to be renamed "when the host is not registered and the cert is not installed" ensuring the cert is fetched
  • A new context block will need to be added for "when the host is not registered and the cert is installed" ensuring that the cert is not fetched (the new behavior you are adding)

You'll likely need to add an allow().to receive(:katello_cert_rpm_installed?).and_return() statements to existing context blocks too now that it will be called and depended on.

Please give that a shot and let me know if you have any questions on the tests.

~Adam

— Reply to this email directly or view it on GitHub https://github.com/chef-partners/redhat-subscription-manager-cookbook/pull/13#issuecomment-176971680 .

[adamleff]

We're here to help, @donwlewis, and appreciate you contributing. Just post here if you get stuck and we'll help you get it over the finish line.

[donwlewis]

OK just got back from vacation so going to try and buckle down on this. Thanks.

On Fri, Jan 29, 2016 at 1:17 PM Adam Leff notifications@github.com wrote:

We're here to help, @donwlewis https://github.com/donwlewis, and appreciate you contributing. Just post here if you get stuck and we'll help you get it over the finish line.

— Reply to this email directly or view it on GitHub https://github.com/chef-partners/redhat-subscription-manager-cookbook/pull/13#issuecomment-176972512 .

[donwlewis]

Adam,

Will this block need to be edited as well?

context 'when satellite_host is provided and host is not registered' do
      let(:remote_file) { chef_run.remote_file('/tmp/chefspec/katello-package.rpm') }

      before do
        allow_any_instance_of(Chef::Resource).to receive(:registered_with_rhsm?).and_return(false)
        allow_any_instance_of(Chef::Resource).to receive(:satellite_host).and_return('sathost')
      end

      it 'fetches the katello cert and config RPM' do
        expect(chef_run).to create_remote_file('/tmp/chefspec/katello-package.rpm')
      end

      it 'installs the katello cert and config RPM' do
        expect(remote_file).to notify('yum_package[katello-ca-consumer-latest]').to(:install)
      end
    end

[adamleff]

@donwlewis that's a good question, and speaks to a larger need to refactor these test a bit so it's clearer what's going on, and ensuring we don't duplicate our work and confuse others. :)

Here's how I'd refactor the tests for the rhsm_register command:

describe 'rhsm_test::unit' do
  describe 'rhsm_register' do
    describe 'fetching the cert RPM' do
      context 'when host is not registered' do
        context 'when using a satellite' do
          context 'when the cert RPM is not yet installed' do
            it 'fetches the cert'
            it 'installs the cert'
          end

          context 'when the cert RPM is already installed' do
            it 'does not fetch the cert'
            it 'does not install the cert'
          end
        end

        context 'when not using a satllite' do
          it 'does not fetch the cert'
          it 'does not install the cert'
        end
      end

      context 'when the host is registered' do
        it 'does not fetch the cert'
        it 'does not install the cert'
      end
    end

    describe 'deleting the fetched package' do
      it 'deletes the file'
    end

    describe 'registering to RHSM' do
      context 'when host is not registered' do
        it 'registers the host'
      end

      context 'when host is registered' do
        it 'does not register the host'
      end
    end

    describe 'installing the katello agent' do
      context 'when installation of the agent is enabled' do
        it 'installs the agent'
      end

      context 'when installation of the agent is disabled' do
        it 'does not install the agent'
      end
    end
  end
end

I think you can take that and run with it, filling in the blanks from there. What do you say? :)

[donwlewis]

That looks good to me.

On Thu, Feb 4, 2016 at 1:50 PM Adam Leff notifications@github.com wrote:

@donwlewis https://github.com/donwlewis that's a good question, and speaks to a larger need to refactor these test a bit so it's clearer what's going on, and ensuring we don't duplicate our work and confuse others. :)

Here's how I'd refactor the tests for the rhsm_register command:

describe 'rhsm_test::unit' do describe 'rhsm_register' do describe 'fetching the cert RPM' do context 'when host is not registered' do context 'when using a satellite' do context 'when the cert RPM is not yet installed' do it 'fetches the cert' it 'installs the cert' end

      context 'when the cert RPM is already installed' do
        it 'does not fetch the cert'
        it 'does not install the cert'
      end
    end

    context 'when not using a satllite' do
      it 'does not fetch the cert'
      it 'does not install the cert'
    end
  end

  context 'when the host is registered' do
    it 'does not fetch the cert'
    it 'does not install the cert'
  end
end

describe 'deleting the fetched package' do
  it 'deletes the file'
end

describe 'registering to RHSM' do
  context 'when host is not registered' do
    it 'registers the host'
  end

  context 'when host is registered' do
    it 'does not register the host'
  end
end

describe 'installing the katello agent' do
  context 'when installation of the agent is enabled' do
    it 'installs the agent'
  end

  context 'when installation of the agent is disabled' do
    it 'does not install the agent'
  end
end

endend

I think you can take that and run with it, filling in the blanks from there. What do you say? :)

— Reply to this email directly or view it on GitHub https://github.com/chef-partners/redhat-subscription-manager-cookbook/pull/13#issuecomment-180066507 .

[donwlewis]

OK Adam, and I apologize for my noobieness :)

This is the code I have so far:

describe 'rhsm_test::unit' do
  describe 'rhsm_register' do
      describe 'fetching the cert RPM' do
        context 'when host is not registered' do
          context 'when using a satellite' do
            context 'when the cert RPM is not yet installed' do
              let(:remote_file) { chef_run.remote_file('/tmp/chefspec/katello-package.rpm') }
              it 'fetches the cert' do
                expect(chef_run).to create_remote_file('/tmp/chefspec/katello-package.rpm')
              end
              it 'installs the cert' do
                expect(remote_file).to notify('yum_package[katello-ca-consumer-latest]').to(:install)
              end
            end

            context 'when the cert RPM is already installed' do
              it 'does not fetch the cert' do
                expect(chef_run).not_to create_remote_file('/tmp/chefspec/katello-package.rpm')
              end
              it 'does not install the cert' do
                expect(remote_file).to notify('yum_package[katello-ca-consumer-latest]').to(:install)
              end
            end
          end

I know I am most likely missing stuff so let me know what I am doing wrong. Again I am very very new to ruby.

[adamleff]

Don't apologize! Everyone is new to Ruby eventually. This is a great time to learn.

You have the correct structure down. What you need to do is mock some of the method calls that Chef is going to do in each of the bottom-level context blocks. In other words, your very first it block/test is in this "context" (or set of conditions):

rhsm_test::unit, rhsm_register, when the host is not registered, when using a satellite, when the cert RPM is not yet installed

Now you need to set up each of those conditions, and the way to do that is by mocking some of those calls to trick the test application into thinking those contexts are true. You do that with allow statements. You can either put those statements in the it blocks directly, or you can put them in a before block so they get executed within that context for each of the tests. My general rule is: put them in the it block if you have one it block - otherwise, use a before block.

So, you're going to want to add a before block before your it block that looks like this:

before do
  allow_any_instance_of(Chef::Resource).to receive(:registered_with_rhsm?).and_return(false)
  allow_any_instance_of(Chef::Resource).to receive(:satellite_host).and_return('sathost')
  allow_any_instance_of(Chef::Resource).to receive(:katello_cert_rpm_installed?).and_return(false)
end

See how each of those allow lines matches to one of the "when" context lines? When your tests are run and those expect lines are evaluated, they'll be evaluated with those conditions mocked.

Then you'll skip to the next context section, copy that before block, but since the next context it "when the cert RPM is already installed", you'll change that katello_cert_rpm_installed? allow line to return true instead of false.

Assuming you have the Chef DK installed, you can execute your tests by running rspec -fd -c (-fd == documentation format, -c == color) and see the outcome of your testing! Fix your tests and move onto the next block.

Let me know if this helps or if it doesn't make any sense.

[donwlewis]

OK Adam. I think I get what you are saying. I will update as soon as I have what I think is the correct code. Thanks again!

On Tue, Feb 9, 2016 at 8:53 AM Adam Leff notifications@github.com wrote:

Don't apologize! Everyone is new to Ruby eventually. This is a great time to learn.

You have the correct structure down. What you need to do is mock some of the method calls that Chef is going to do in each of the bottom-level context blocks. In other words, your very first it block/test is in this "context" (or set of conditions):

rhsm_test::unit, rhsm_register, when the host is not registered, when using a satellite, when the cert RPM is not yet installed

Now you need to set up each of those conditions, and the way to do that is by mocking some of those calls to trick the test application into thinking those contexts are true. You do that with allow statements. You can either put those statements in the it blocks directly, or you can put them in a before block so they get executed within that context for each of the tests. My general rule is: put them in the it block if you have one it block - otherwise, use a before block.

So, you're going to want to add a before block before your it block that looks like this:

before do allow_any_instance_of(Chef::Resource).to receive(:registered_with_rhsm?).and_return(false) allow_any_instance_of(Chef::Resource).to receive(:satellite_host).and_return('sathost') allow_any_instance_of(Chef::Resource).to receive(:katello_cert_rpm_installed?).and_return(false) end

See how each of those allow lines matches to one of the "when" context lines? When your tests are run and those expect lines are evaluated, they'll be evaluated with those conditions mocked.

Then you'll skip to the next context section, copy that before block, but since the next context it "when the cert RPM is already installed", you'll change that katello_cert_rpm_installed? allow line to return true instead of false.

Assuming you have the Chef DK installed, you can execute your tests by running rspec -fd -c (-fd == documentation format, -c == color) and see the outcome of your testing! Fix your tests and move onto the next block.

Let me know if this helps or if it doesn't make any sense.

— Reply to this email directly or view it on GitHub https://github.com/chef-partners/redhat-subscription-manager-cookbook/pull/13#issuecomment-181954399 .

[donwlewis]

OK Adam, just another check before I finish out:

          context 'when using a satellite' do
            context 'when satellite host is nil' do
              it 'does not fetch the katello RPM' do
                allow_any_instance_of(Chef::Resource).to receive(:satellite_host).and_return(nil)
                expect(chef_run).not_to create_remote_file('/tmp/chefspec/katello-package.rpm')
              end
            end
            context 'when satellite host is provided' do
              context 'when the cert RPM is not yet installed' do
                let(:remote_file) { chef_run.remote_file('/tmp/chefspec/katello-package.rpm') }

                before do
                  allow_any_instance_of(Chef::Resource).to recieve(:katello_cert_rpm_installed?).and_return(false)
                end

                it 'fetches the cert' do
                  expect(chef_run).to create_remote_file('/tmp/chefspec/katello-package.rpm')
                end

                it 'installs the cert' do
                  expect(remote_file).to notify('yum_package[katello-ca-consumer-latest]').to(:install)
                end
              end

              context 'when the cert RPM is already installed' do
                before do
                  allow_any_instance_of(Chef::Resource).to recieve(:katello_cert_rpm_installed?).and_return(true)
                end

                it 'does not fetch and install the cert' do
                  expect(chef_run).not_to create_remote_file('/tmp/chefspec/katello-package.rpm')
                  expect(remote_file).not_to notify('yum_package[katello-ca-consumer-latest]').to(:install)
                end
              end
            end
          end

I added the context for when the satellite host was nil. Not sure if it is still appropriate there or not.

[adamleff]

Seems fine to me! Finish out your tests and push them up to the branch and let's see how they all look.

[donwlewis]

OK Adam, I ran rspec after finishing it up, and am really not sure how to read the output. If you have a chance, can you review the attached file, and let me know how it looks? rspec_out.txt

[adamleff]

@donwlewis Basically, you're getting a lot of errors when running the chefspec tests that, to me, almost look like Chefspec isn't getting loaded properly. I'm not a Windows guru so I'm not sure how to guide you on fixing it.

My recommendation at this point is to push up what you have to the branch and let's see if Travis detects errors. Any errors we see, I'll try and fix them myself and we can go from there. Fair enough?

[donwlewis]

OK pushed what I have. I will try to get my linux VM up and running to continue helping out. Thanks so much for all the help!

On Fri, Feb 12, 2016 at 12:06 PM Adam Leff notifications@github.com wrote:

@donwlewis https://github.com/donwlewis Basically, you're getting a lot of errors when running the chefspec tests that, to me, almost look like Chefspec isn't getting loaded properly. I'm not a Windows guru so I'm not sure how to guide you on fixing it.

My recommendation at this point is to push up what you have to the branch and let's see if Travis detects errors. Any errors we see, I'll try and fix them myself and we can go from there. Fair enough?

— Reply to this email directly or view it on GitHub https://github.com/chef-partners/redhat-subscription-manager-cookbook/pull/13#issuecomment-183470168 .

[donwlewis]

Hmmm. I must have some residual configs from my old github account. Anyway, I not really understanding what is causing the errors we are seeing. Seems to me it needs to be tested on a RHEL system?

[adamleff]

@donwlewis no, you should be able to unit test on any platform where ruby/rspec works. It looks like there might be a syntax issue causing ChefSpec to not properly load the cookbook. I'm taking a look at this now and will fix it up. Good job fixing the syntax errors that was causing the initial rubocop failures you saw after your first push.

[donwlewis]

Thanks Adam. I think I understand how the process works better now. Appreciate all your help with this. Hopefully I can contribute more as I start learning this more. Is there a way to more verbosity to maybe pin point where the issue may be?

On Wed, Feb 17, 2016 at 7:29 AM Adam Leff notifications@github.com wrote:

@donwlewis https://github.com/donwlewis no, you should be able to unit test on any platform where ruby/rspec works. It looks like there might be a syntax issue causing ChefSpec to not properly load the cookbook. I'm taking a look at this now and will fix it up. Good job fixing the syntax errors that was causing the initial rubocop failures you saw after your first push.

— Reply to this email directly or view it on GitHub https://github.com/chef-partners/redhat-subscription-manager-cookbook/pull/13#issuecomment-185255239 .

[adamleff]

@donwlewis: there were a few issues with your test changes, but all-in-all, it was an awesome first attempt:

• The definition of chef_run was accidentally removed in the describe 'rhsm_register block
• A few method mocks (i.e. the allow_any_instance_of commands) were missing which was causing ChefSpec to actually try and run subscription_manager rather than using mocked responses
• Some of the context blocks (i.e. context 'rhsm_repo') got outdented one level, outside of the describe 'rhsm_test::unit' block, which caused the chef_run definitions to fail in those blocks (because the "described_recipe" no longer existed).

I fixed those up, added back some whitespace (for readability) that got removed, and it appears all the testing is passing. I squashed the commits down (to keep the branch as clean as possible without removing context/insight) and opened a new PR, #17, which will supersede this one. Once the travis tests pass on that one, I'll merge it in and release a new cookbook version to the supermarket later today.

Thanks again for your contribution!

[donwlewis]

Thanks again Adam. As I get better at doing this stuff, hopefully I will be able to contribute more. I need to get better at testing this stuff. Thanks again for all the help!

On Wed, Feb 17, 2016 at 8:03 AM Adam Leff notifications@github.com wrote:

Closed #13 https://github.com/chef-partners/redhat-subscription-manager-cookbook/pull/13 .

— Reply to this email directly or view it on GitHub https://github.com/chef-partners/redhat-subscription-manager-cookbook/pull/13#event-553774192 .