Use the new setup script argument types #14313
Conversation
carbonin
force-pushed
the
use_new_setup_script
branch
from
47b40ab
to
b6cd224
Compare
lib/embedded_ansible.rb
Outdated
| with_inventory_file do |inventory_file_path| | ||
| AwesomeSpawn.run!(SETUP_SCRIPT, :params => params.merge(:i => inventory_file_path)) | ||
| params = [[:e, json_extra_vars], [:i, inventory_file_path]] << "--" << ["--skip-tags=", exclude_tags] |
There was a problem hiding this comment.
I think this might read easier split out even though you'd get more lines added 😢
params = []
params << [:e, json_extra_vars]
params << [:i, inventory_file_path]
params << "--"
params << ["--skip-tags=", exclude_tags]Otherwise, it might be easy to miss the "--"
Also, this way you don't have to see [ and mentally search for the closing ].
Does this change require a specific version of the setup script? Do we need to worry about which version of the setup script we're going to get? |
|
@jrafanie No, we should be fine, this is the version that will come with 3.1.2 |
if 💚
carbonin
force-pushed
the
use_new_setup_script
branch
from
c70de25
to
1e73fab
Compare
lib/embedded_ansible.rb
Outdated
| params << [:e, json_extra_vars] | ||
| params << [:i, inventory_file_path] | ||
| params << "--" | ||
| params << ["--skip-tags=", exclude_tags] |
There was a problem hiding this comment.
I think this can be done a little cleaner...
params = {
:e => json_extra_vars,
:i => inventory_file_path,
"--" => nil,
"--skip-tags=" => exclude_tags
}Might I also recommend long-form tags for e and i for readability?
There was a problem hiding this comment.
Will this ensure that order is preserved? That's why I moved from hash to array.
There was a problem hiding this comment.
Also, just double checked, there are no long form options for -e and -i.
There was a problem hiding this comment.
Interesting, so the setup script doesn't take long options, but the ansible-playbook command does. So we could do:
params = {
"--" => nil,
:extra_vars= => json_extra_vars,
:inventory= => inventory_file_path,
:skip_tags= => exclude_tags
}There was a problem hiding this comment.
It's up to you what you think is preferable.
spec/lib/embedded_ansible_spec.rb
Outdated
| @@ -228,12 +228,16 @@ | |||
| it "generates new passwords with no passwords set" do | |||
| expect(described_class).to receive(:generate_database_authentication).and_return(double(:userid => "awx", :password => "databasepassword")) | |||
| expect(AwesomeSpawn).to receive(:run!) do |script_path, options| | |||
| params = options[:params] | |||
| inventory_file_contents = File.read(params[:i]) | |||
| e, i, dashes, skip_tags = options[:params] | |||
There was a problem hiding this comment.
Same here...Instead of e and i, can we have full names for readability?
A change was made to pass options that come to the setup script after a "--" straight through, unaltered to the ansible-playbook command. This is how we will get access to the --tags and --skip-tags options that we need.
carbonin
force-pushed
the
use_new_setup_script
branch
from
1e73fab
to
b12c8fd
Compare
|
Checked commit carbonin@b12c8fd with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
Merging, comments appear to be addressed. |
A change was made to pass options that come to the setup script after a "--" straight through, unaltered to the
ansible-playbookcommand.This is how we will get access to the --tags and --skip-tags options that we need.