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

Allow loading of external fact hashes #62

Merged
merged 1 commit into from
Oct 18, 2017

Conversation

logicminds
Copy link
Contributor

  • previously there was no way to supply facterdb with
    external facts that contain sensitive information. This
    fixes that by allowing the user to set an environment variable
    that take precedence over the facts from this gem.

@coveralls
Copy link

coveralls commented Oct 13, 2017

Coverage Status

Coverage increased (+6.5%) to 80.0% when pulling f56468c on nwops:external_facts into 369c1ec on camptocamp:master.

2 similar comments
@coveralls
Copy link

coveralls commented Oct 13, 2017

Coverage Status

Coverage increased (+6.5%) to 80.0% when pulling f56468c on nwops:external_facts into 369c1ec on camptocamp:master.

@coveralls
Copy link

coveralls commented Oct 13, 2017

Coverage Status

Coverage increased (+6.5%) to 80.0% when pulling f56468c on nwops:external_facts into 369c1ec on camptocamp:master.

@coveralls
Copy link

coveralls commented Oct 14, 2017

Coverage Status

Coverage increased (+6.5%) to 80.0% when pulling fd30a03 on nwops:external_facts into 369c1ec on camptocamp:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+6.5%) to 80.0% when pulling c3b14c3 on nwops:external_facts into 369c1ec on camptocamp:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+6.5%) to 80.0% when pulling c3b14c3 on nwops:external_facts into 369c1ec on camptocamp:master.

@coveralls
Copy link

coveralls commented Oct 14, 2017

Coverage Status

Coverage increased (+6.5%) to 80.0% when pulling c3b14c3 on nwops:external_facts into 369c1ec on camptocamp:master.

@coveralls
Copy link

coveralls commented Oct 14, 2017

Coverage Status

Coverage increased (+4.02%) to 77.551% when pulling 22d0c11 on nwops:external_facts into 369c1ec on camptocamp:master.

@mcanevet
Copy link
Member

LGTM. @DavidS @rodjek what do you think about this PR?

Gemfile Outdated
@@ -7,3 +7,7 @@ if facterversion = ENV['FACTER_GEM_VERSION']
else
gem 'facter', :require => false
end

group :dev do
gem 'pry'
Copy link
Member

Choose a reason for hiding this comment

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

Can you add this to the gemspec as a development dependency instead of here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added to gemspec

Copy link
Contributor

Choose a reason for hiding this comment

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

then you can remove it here?

lib/facterdb.rb Outdated
def self.external_fact_files(fact_paths = ENV['FACTERDB_SEARCH_PATH'])
fact_paths ||= ''
return [] if fact_paths.empty?
paths = fact_paths.split(',').map do |fact_path|
Copy link
Member

Choose a reason for hiding this comment

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

Rather than splitting by ,, can we change this to File::PATH_SEPARATOR which will be a bit more idiomatic to users (: on Linux/BSD/etc, ; on Windows)

lib/facterdb.rb Outdated
end
File.join(fact_path.strip, '**', '*.facts')
end.compact
Dir.glob(paths)
Copy link
Member

Choose a reason for hiding this comment

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

Dir.glob behaves badly when passed Windows style backslash delimited paths, so I'd recommend adding the following line to the above map before the final File.join to convert the backslashes.

fact_path = fact_path.gsub(File::ALT_SEPARATOR, File::SEPARATOR) if File::ALT_SEPARATOR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are great suggestions. Fixed in latest push.

@coveralls
Copy link

Coverage Status

Coverage increased (+4.5%) to 78.0% when pulling e95cb33 on nwops:external_facts into 369c1ec on camptocamp:master.

1 similar comment
@coveralls
Copy link

coveralls commented Oct 14, 2017

Coverage Status

Coverage increased (+4.5%) to 78.0% when pulling e95cb33 on nwops:external_facts into 369c1ec on camptocamp:master.

@logicminds
Copy link
Contributor Author

This is ready to go after adding the suggested fixes. @rodjek

@DavidS
Copy link
Contributor

DavidS commented Oct 16, 2017

I like the idea, and given that there is already https://github.com/mcanevet/rspec-puppet-facts#set-global-custom-facts there is surely a need for it, too. In the light of #61 , I wonder if it wouldn't make sense to extend this even further and make the first/default entry of FACTERDB_SEARCH_PATH a special value, like BUILTIN, so that folks can override the DB contents completely, if they want?

@logicminds
Copy link
Contributor Author

@DavidS we can do something like this:

# @return [Array[String]] -  list of all files found in the default facterdb facts path
  def self.default_fact_files
    proj_root = File.join(File.dirname(File.dirname(__FILE__)))
    facts_dir = ENV['FACTERDB_DEFAULT_SEARCH_PATH'] || File.expand_path(File.join(proj_root, 'facts'))
    Dir.glob(File.join(facts_dir, "**", '*.facts'))
  end

@logicminds
Copy link
Contributor Author

Also now that I look at this. I am going to change FACTERDB_SEARCH_PATH to FACTERDB_SEARCH_PATHS since we can supply multiple paths.

@DavidS
Copy link
Contributor

DavidS commented Oct 17, 2017

*_PATH sounds fine to me.

Re the default search path, I've spent some more time on that, and I think we should optimize strongly for the default case of folks using the built-in facts. What about return [] if ENV['FACTERDB_SKIP_DEFAULTDB'] == 'yes', adding "Set FACTERDB_SKIP_DEFAULTDB=yes to instruct facterdb to look at its built-in facts, if you want to completely replace which facts are used." to the README.

  * previously there was no way to supply facterdb with
    external facts that contain sensitive information.  This
    fixes that by allowing the user to set an environment variable
    that take precedence over the facts from this gem.
@coveralls
Copy link

coveralls commented Oct 17, 2017

Coverage Status

Coverage increased (+5.7%) to 79.245% when pulling 695e103 on nwops:external_facts into 369c1ec on camptocamp:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+5.7%) to 79.245% when pulling 695e103 on nwops:external_facts into 369c1ec on camptocamp:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+5.7%) to 79.245% when pulling 695e103 on nwops:external_facts into 369c1ec on camptocamp:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+5.7%) to 79.245% when pulling 695e103 on nwops:external_facts into 369c1ec on camptocamp:master.

@logicminds
Copy link
Contributor Author

I have added @DavidS suggestions and updated the readme to include this additional info on how to skip the default database. I think this feature will be a big hit.

@@ -6,4 +6,4 @@ if facterversion = ENV['FACTER_GEM_VERSION']
gem 'facter', facterversion, :require => false
else
gem 'facter', :require => false
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

:-(

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 didn't want to get rubocop involved as it would have changed a whole lot of code. But now that this is merged, let the games begin. Also we need to bump the version.

Copy link
Contributor

Choose a reason for hiding this comment

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

rubocop++, and I would suggest the PDK template, obviously. But final say is with @mcanevet and @raphink .

the version gets bumped during release-prep in accordance with the changes in the release.

Copy link
Member

Choose a reason for hiding this comment

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

@DavidS I didn't have time to check PDK yet but feel free to add this in this module. Ping me onde it's OK for release.

@DavidS DavidS merged commit 6dbac7e into voxpupuli:master Oct 18, 2017
@DavidS DavidS changed the title Fixes #27 - allow loading of external fact hashes Allow loading of external fact hashes Oct 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants