Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an apply matcher #115

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Oct 25, 2022

This allows users to specify code as:

describe 'motd class' do
  subject { 'include motd' }

  # Just run it once
  it { is_expected.to apply }

  # The same, but run it twice
  it { is_expected.to apply.idempotently }
end

This gives the full output of puppet apply, rather than just the last 10 lines. It also tells the user if it failed on the first or second run.

Additionally, users can define a failure_context method to run an additional command and output it. The main motivation for this is when puppet apply doesn't provide any useful output on the failure. For example, when a service fails to start Puppet does provide the journal output, but sometimes it only logs errors to its own files (looking at you Apache). This command is only executed if the Puppet run failed to apply.

describe 'motd class' do
  subject { 'include motd' }

  def failure_context
    'cat /etc/motd'
  end

  it { is_expected.to apply.idempotently }
end

I'm submitting this for feedback now. I have some additional work with example groups so you can make it even easier.

@ekohl
Copy link
Member Author

ekohl commented Oct 28, 2022

@trevor-vaughan I'd like to get your opinion on this since AFAIK you're a significant user of Beaker.

My goal with this was to provide proper debugging of failures. By using a matcher it shouldn't show a track trace anymore (which is kind of pointless anyway) but instead the actual output. Also where it failed.

I also wanted a hook to show more output, but I think the failure_context may be a bit too well hidden. Not sure what a good way is.

@trevor-vaughan
Copy link
Contributor

trevor-vaughan commented Oct 28, 2022 via email

@ekohl ekohl force-pushed the add-apply-matcher branch from 70c362a to e6be805 Compare October 28, 2022 12:28
This allows users to specify code as:

    describe 'motd class' do
      subject { 'include motd' }

      # Just run it once
      it { is_expected.to apply }

      # The same, but run it twice
      it { is_expected.to apply.idempotently }
    end

This gives the full output of puppet apply, rather than just the last 10
lines. It also tells the user if it failed on the first or second run.

Additionally, users can define a failure_context method to run an
additional command and output it. The main motivation for this is when
puppet apply doesn't provide any useful output on the failure. For
example, when a service fails to start Puppet does provide the journal
output, but sometimes it only logs errors to its own files (looking at
you Apache). This command is only executed if the Puppet run failed to
apply.

    describe 'motd class' do
      subject { 'include motd' }

      def failure_context
        'cat /etc/motd'
      end

      it { is_expected.to apply.idempotently }
    end
@ekohl ekohl force-pushed the add-apply-matcher branch from e6be805 to e298601 Compare October 28, 2022 12:36
@ekohl
Copy link
Member Author

ekohl commented Oct 28, 2022

Tests now fail because Puppet is not installed. I'll deal with that later. It at least shows how it should be used now.

@trevor-vaughan
Copy link
Contributor

@ekohl Took a deeper look at how we're using things and here's my feedback:

  • I think that this should probably live in beaker-puppet since you can (and I do) use beaker without puppet very successfully
    under the hood. When the tests are more verbose about what's going on, ramp up time is shorter.
  • Honestly, I'm not seeing a huge benefit over the current apply_manifest_on pattern below outside of the improved error output.
    • I think that adding the improved error output to the default version is probably a good idea. We tend to run in verbose mode all the time just to see everything and it would be great to not have to do that.
    • Maybe change it so that there is a boolean to flip that defaults to verbose for puppet apply?
  • If we move forward with this, I would definitely make the name puppet specific since apply is too generic given that beaker can be used without puppet
  • Caveats about my response:
    • We don't use serverspec at all because it doesn't support multi-node setups well the way it's implemented in Beaker
    • Also, my experience using serverspec is that it makes things harder to understand for new users. Too much magic going on
let(:manifest) { whatever }
let(:hieradata) {{ whatever }}

hosts.each do |host|
  it 'should apply' do
    set_hieradata_on(host, hieradata)
    apply_manifest_on(host, manifest)
  end

  it 'should be idempotent' do
    apply_manifest_on(host, manifest, catch_changes: true)
  end
end

@jhoblitt
Copy link
Member

jhoblitt commented Nov 4, 2022

I like this concept. I vote for putting an experimental warning on it and shipping it to get some real world usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants