-
Notifications
You must be signed in to change notification settings - Fork 21
(FM-2406) Enable tests in CI to install from staging forge #111
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
(FM-2406) Enable tests in CI to install from staging forge #111
Conversation
Looks good based on the documentation and what I know of the code. I'll run it locally tonight and update tomorrow. |
@@ -53,7 +53,15 @@ | |||
on agent, puppet("module install #{dep}") | |||
end | |||
on agent, "git clone https://github.com/puppetlabs/puppetlabs-mount_iso #{target}/mount_iso" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this no longer be necessary if we are installing from forge? Or is this not a dependency of this module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an unpublished module still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough..
@justinstoller what did you find out? |
@@ -96,3 +96,95 @@ def base_install(sql_version) | |||
step "Install Microsoft SQL #{sql_version} on the agent #{host.node_name} before running any tests" | |||
install_sqlserver(host, {:features => 'SQL'}) | |||
end | |||
|
|||
def install_ca_certs(host) | |||
GEOTRUST_GLOBAL_CA = <<-EOM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't assign to a constant within a method:
15:17:02 hobo:puppetlabs-sqlserver justin (merge/maint/master/FM-2406_install_from_forge *%) ⭑ ruby --version
ruby 1.9.3p547 (2014-05-14) [x86_64-darwin14.1.0]
15:11:47 hobo:puppetlabs-sqlserver justin (merge/maint/master/FM-2406_install_from_forge %) ⭑ ruby -c spec/sql_testing_helpers.rb
spec/sql_testing_helpers.rb:101: dynamic constant assignment
GEOTRUST_GLOBAL_CA = <<-EOM
^
spec/sql_testing_helpers.rb:124: dynamic constant assignment
USERTRUST_NETWORK_CA = <<-EOM
^
spec/sql_testing_helpers.rb:153: dynamic constant assignment
EQUIFAX_CA = <<-EOM
^
In testing locally, moving these to the top of the file resolved the issue. Though the scope those constants are then defined in is a little fuzzy to me. Providing an explicit test helpers modules namespace would probably be the best way to go.
I don't know if it's in scope or not, but idiomatically (with RSpec at least) supporting test utilities are put in |
EOM | ||
|
||
|
||
step "Installing Geotrust CA cert" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
step
, test_name
, and teardown
are methods that help "structure" a Beaker test and are similar (though not strictly analogous) to it
, describe
and after
in RSpec. Consequently those Beaker methods shouldn't be used in RSpec (is an error in most cases) and instead their RSpec cousins should be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justinstoller I agree. However step
is already used in this repository. If we would like to remove it I think that should be ticketed as separate work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will pass without that work. See the commit message I put up in puppetlabs-toy-chest/puppetlabs-catalog_preview#42
fb25555
to
32882c4
Compare
32882c4
to
40d3c86
Compare
Supersedes #89.