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

(BKR-1342) Import ci rake tasks and install utils #26

Conversation

melissa
Copy link
Contributor

@melissa melissa commented Feb 2, 2018

This commit moves all ci acceptance rake tasks and install/setup
utilities out of the puppet repo and into the beaker-puppet repo. This
will allow us to reuse all of this code across multiple components of
puppet-agent. The goal is to reduce the number of places we need to
duplicate code across different code bases. With this change, any future
updates or overhauls will be consistent across all our projects.

@melissa
Copy link
Contributor Author

melissa commented Feb 2, 2018

This is still blocked on a few updated to puppet, so it's not yet ready to be merged

@melissa melissa force-pushed the ticket/master/BKR-1342-puppet-acceptance-utilities branch from 7d7fbe3 to 7a51054 Compare February 2, 2018 22:03
@melissa melissa changed the title [WIP] (BKR-1342) Import ci rake tasks and install utils (BKR-1342) Import ci rake tasks and install utils Feb 2, 2018
@melissa
Copy link
Contributor Author

melissa commented Feb 2, 2018

This PR is based on changes made in puppetlabs/puppet#6550

return true
end

def ruby_command(host)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like these here, but I was unsure where to drop them. They are utilities that are only used by tests defined in common.rb, gem.rb, aio.rb, etc. They are not referenced by a component when running tests, as are the rest of the methods in this file. I think it would be good to move these to a different file, but I wasn't sure the best place for that

@melissa
Copy link
Contributor Author

melissa commented Feb 5, 2018

@kevpl I just realized you already have a lot of this code in the acceptance directory of beaker-puppet (https://github.com/puppetlabs/beaker-puppet/tree/master/acceptance). Was that intended to be used by projects like puppet and hiera to run acceptance tests?

def install_cumulus_modules
test_name "Install Cumulus Modules" do
platforms = hosts.map{|val| val[:platform]}
skip_test "No cumulus hosts present" unless platforms.any? { |val| /cumulus/ =~ val }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshcooper it looks like this skip is causing the top level test (https://github.com/puppetlabs/puppet/pull/6602/files#diff-62e97df027f65c15385cb6a34498e2acR1) to skip, unless I'm missing something? Is this something that happens with nested tests?

  Install Cumulus Modules

    No cumulus hosts present
Begin teardown
End teardown
Warning: setup/aio.rb skipped in 91.94 seconds
      Test Suite: pre_suite @ 2018-02-06 11:10:42 -0800

      - Host Configuration Summary -


              - Test Case Summary for suite 'pre_suite' -
       Total Suite Time: 91.94 seconds
      Average Test Time: 91.94 seconds
              Attempted: 1
                 Passed: 0
                 Failed: 0
                Errored: 0
                Skipped: 1
                Pending: 0
                  Total: 1

      - Specific Test Case Status -

Failed Tests Cases:
Errored Tests Cases:
Skipped Tests Cases:
  Test Case setup/aio.rb
Pending Tests Cases:

No tests to run for suite 'tests'
No tests to run for suite 'post_suite'
No tests to run for suite 'pre_cleanup'
Cleanup: cleaning up after successful run

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright, I got this figured out

@melissa melissa force-pushed the ticket/master/BKR-1342-puppet-acceptance-utilities branch from e4202af to dd7dc91 Compare February 7, 2018 01:31
@kevpl
Copy link
Contributor

kevpl commented Feb 8, 2018

@melissa to this question:

@kevpl I just realized you already have a lot of this code in
the acceptance directory of beaker-puppet
(https://github.com/puppetlabs/beaker-puppet/tree/master/acceptance).
Was that intended to be used by projects like puppet and hiera to
run acceptance tests?

No, it wasn't, it was put there just to help beaker-puppet do its own acceptance testing. However, we'd much rather have a "true" (meaning not in beaker-puppet's acceptance helpers) DSL method that both you & we can use in testing. So my recommendation would be that we switch beaker-puppet's testing to use your versions of the methods & delete beaker-puppet-acceptance's copy of them.

I understand that this is additional work for what is already potentially a big glob of work, so I don't think this needs to necessarily be done before this PR is merged. If you'd like, we can ticket this work for BKR & hopefully be able to schedule this soon with a goalie. @melissa would you like to create the ticket since your head is in this right now, or should I do that?

@melissa melissa added the blocked label Feb 8, 2018
@melissa
Copy link
Contributor Author

melissa commented Feb 8, 2018

@kevpl created https://tickets.puppetlabs.com/browse/BKR-1424 for that work. Thank you!

Also, this is blocked because of a bug we found where beaker subcommands don't work well when passing in additional options. BKR-1422 was created to cover this particular bug.

sh('beaker', command.to_s, *argv)
end

def beaker_suite(type)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshcooper this is what I have so far. Not necessarily a final version, but at least it's proof this is working!

@melissa melissa force-pushed the ticket/master/BKR-1342-puppet-acceptance-utilities branch from 1878517 to 3e383f0 Compare February 16, 2018 01:09
# Methods that help you interact with rake during ci setup
module RakeHelpers
class << self
def load_tasks(beaker_root = '/Users/melissa/beaker-puppet')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make it a required parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh shoot, I meant to fix this, but I totally forgot. Thank you! I don't think we need to make it required, since we should be able to introspect where beaker-puppet is being run. I do that for the setup tests

@melissa melissa force-pushed the ticket/master/BKR-1342-puppet-acceptance-utilities branch from 3e383f0 to 182e1c3 Compare February 23, 2018 18:04
# Methods that help you interact with rake during ci setup
module RakeHelpers
class << self
def load_tasks(beaker_root = File.dirname(__dir__))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshcooper fixed!

@melissa melissa force-pushed the ticket/master/BKR-1342-puppet-acceptance-utilities branch from 182e1c3 to 429a9aa Compare March 12, 2018 21:50
@melissa melissa removed the blocked label Mar 12, 2018
@melissa
Copy link
Contributor Author

melissa commented Mar 12, 2018

I tested aio, gem, and git. Aio and gem both succeeded. Git failed, but in an expected way and should not be a blocker.

Copy link
Contributor

@kevpl kevpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshcooper any concerns you'd like to raise, or can I proceed with merging & releasing these changes?

tasks/ci.rake Outdated
task :quick => ['ci:check_env', 'ci:gen_hosts'] do
ENV['TESTS'] = get_test_sample.join(",")
Rake::Task["ci:test:aio"].invoke
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ci:test:quick task is specific to the puppet component, e.g. the get_test_sample method returns tests that only live in puppet. How about keeping this task (and related helper methods) in puppet's Rakefile?

This commit moves all ci acceptance rake tasks and install/setup
utilities out of the puppet repo and into the beaker-puppet repo. This
will allow us to reuse all of this code across multiple components of
puppet-agent. The goal is to reduce the number of places we need to
duplicate code across different code bases. With this change, any future
updates or overhauls will be consistent across all our projects.
@melissa melissa force-pushed the ticket/master/BKR-1342-puppet-acceptance-utilities branch from 429a9aa to eb2a9a5 Compare March 16, 2018 17:25
@melissa
Copy link
Contributor Author

melissa commented Mar 16, 2018

@joshcooper I removed the ci:test:quick task and associated utilities

@ericldelaney did you want me to add a check for if --preserve-hosts=always is present in OPTIONS and skip beaker destroy if it is?

@ericldelaney
Copy link

@melissa it would be nice if at some point we honored --preserve-hosts correctly. It would save me from having to create a failing test to run against so that it leaves the machines. (shrug)

Copy link
Contributor

@joshcooper joshcooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's file a ticket about adding back preserve hosts.

@joshcooper joshcooper merged commit adf1547 into puppetlabs:master Mar 16, 2018
@melissa
Copy link
Contributor Author

melissa commented Mar 16, 2018

ticket filed https://tickets.puppetlabs.com/browse/BKR-1443 feel free to add more detail

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.

4 participants