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

Handle directories with vars as well #82

Merged
merged 1 commit into from Nov 3, 2016

Conversation

Projects
None yet
2 participants
@agx
Contributor

agx commented Sep 28, 2016

group_vars and host_vars can be arbitrary named files in directories
named like the host or group. Handle this case as well.

@volanja

Please check code.
and Could you add some test case?

Show outdated Hide outdated lib/ansible_spec/load_ansible.rb
vars_file = path
if check_no_ext && !File.exist?(vars_file)
vars_file = path+".yml"
end
if File.exist?(vars_file)
yaml = YAML.load_file(vars_file)
vars = merge_variables(vars, yaml)
if File.directory?(vars_file)

This comment has been minimized.

@volanja

volanja Sep 29, 2016

Owner

Could you add some test case?

@volanja

volanja Sep 29, 2016

Owner

Could you add some test case?

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Sep 29, 2016

Contributor

On Thu, Sep 29, 2016 at 08:55:59AM -0700, volanja wrote:

volanja requested changes on this pull request.

Please check code.
and Could you add some test case?

@@ -301,14 +301,20 @@ def self.get_hash_behaviour()

param: flag to extention

true: .yml extension is optional

false: must have .yml extention

  • def self.load_vars_file(vars, path, check_no_ext = false)
  • def self.load_vars_file(vars, path, check_no_ext = false, allow_dir = false)

allow_dir isn't use in this function.
Is this necessary?

 vars_file = path
 if check_no_ext && !File.exist?(vars_file)
   vars_file = path+".yml"
 end
 if File.exist?(vars_file)
  •  yaml = YAML.load_file(vars_file)
    
  •  vars = merge_variables(vars, yaml)
    
  •  if File.directory?(vars_file)
    

Could you add some test case?

Can you suggest a file to add the test?

Contributor

agx commented Sep 29, 2016

On Thu, Sep 29, 2016 at 08:55:59AM -0700, volanja wrote:

volanja requested changes on this pull request.

Please check code.
and Could you add some test case?

@@ -301,14 +301,20 @@ def self.get_hash_behaviour()

param: flag to extention

true: .yml extension is optional

false: must have .yml extention

  • def self.load_vars_file(vars, path, check_no_ext = false)
  • def self.load_vars_file(vars, path, check_no_ext = false, allow_dir = false)

allow_dir isn't use in this function.
Is this necessary?

 vars_file = path
 if check_no_ext && !File.exist?(vars_file)
   vars_file = path+".yml"
 end
 if File.exist?(vars_file)
  •  yaml = YAML.load_file(vars_file)
    
  •  vars = merge_variables(vars, yaml)
    
  •  if File.directory?(vars_file)
    

Could you add some test case?

Can you suggest a file to add the test?

@volanja

This comment has been minimized.

Show comment
Hide comment
@volanja
Owner

volanja commented Sep 29, 2016

@volanja volanja added this to the v0.2.16 milestone Sep 29, 2016

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Sep 29, 2016

Contributor

On Thu, Sep 29, 2016 at 09:07:54AM -0700, volanja wrote:

Please add in spec/get_variable_spec.rb

Thanks for the pointer. Test added!

Contributor

agx commented Sep 29, 2016

On Thu, Sep 29, 2016 at 09:07:54AM -0700, volanja wrote:

Please add in spec/get_variable_spec.rb

Thanks for the pointer. Test added!

Handle directories with vars as well
group_vars and host_vars can be arbitrary named files in directories
named like the host or group. Handle this case as well.

@volanja volanja merged commit ee61e06 into volanja:master Nov 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@volanja

This comment has been minimized.

Show comment
Hide comment
@volanja

volanja Nov 3, 2016

Owner

Sorry for too late response.
I merged it.
Thank you for your contribution.

Owner

volanja commented Nov 3, 2016

Sorry for too late response.
I merged it.
Thank you for your contribution.

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Nov 3, 2016

Contributor

Thanks and no worries. Any chance #83 can be pulled in as well?

Contributor

agx commented Nov 3, 2016

Thanks and no worries. Any chance #83 can be pulled in as well?

@volanja

This comment has been minimized.

Show comment
Hide comment
@volanja

volanja Nov 3, 2016

Owner

I merged #83
it will release at v0.2.16

Owner

volanja commented Nov 3, 2016

I merged #83
it will release at v0.2.16

volanja added a commit that referenced this pull request Nov 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment