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.

Sign up for GitHub

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

Jump to bottom

PDK::Util.in_module_root? can give a false positive result #768

Closed
logicminds opened this issue Oct 8, 2019 · 5 comments · Fixed by #783
Closed

PDK::Util.in_module_root? can give a false positive result #768

logicminds opened this issue Oct 8, 2019 · 5 comments · Fixed by #783
Assignees
@glennsarti
Labels
bug
Milestone
November 2019

Comments

@logicminds
Copy link
Contributor

Describe the bug
The in_module_root? function reports true whenever a lib directory is found. It uses
the following folders to determine if the current directory is indeed a module.

However, just because a folder has a lib directory, does not mean it is a module and does not also mean it has a Gemfile either.

To Reproduce

  1. Create a directory mkdir -p /tmp/test/lib
  2. cd /tmp/test
  3. run pdk console or possible any other pdk command

Expected behavior
PDK should return the correct boolean value when the current directory is not a module.

This might mean that instead of looking for lib, we look for lib/puppet.

Additional context

pdk console
pdk (WARN): Module fixtures not found, please run pdk bundle exec rake spec_prep.
pdk (WARN): Unable to determine Puppet version for module: no metadata.json present in module.
/opt/puppetlabs/pdk/private/ruby/2.4.5/lib/ruby/gems/2.4.0/gems/bundler-2.0.2/lib/bundler.rb:431:in `initialize': no implicit conversion of nil into String (TypeError)
	from /opt/puppetlabs/pdk/private/ruby/2.4.5/lib/ruby/gems/2.4.0/gems/bundler-2.0.2/lib/bundler.rb:431:in `open'
	from /opt/puppetlabs/pdk/private/ruby/2.4.5/lib/ruby/gems/2.4.0/gems/bundler-2.0.2/lib/bundler.rb:431:in `block in read_file'
	from /opt/puppetlabs/pdk/private/ruby/2.4.5/lib/ruby/gems/2.4.0/gems/bundler-2.0.2/lib/bundler/shared_helpers.rb:119:in `filesystem_access'
	from /opt/puppetlabs/pdk/private/ruby/2.4.5/lib/ruby/gems/2.4.0/gems/bundler-2.0.2/lib/bundler.rb:430:in `read_file'
	from /opt/puppetlabs/pdk/private/ruby/2.4.5/lib/ruby/gems/2.4.0/gems/pdk-1.14.0.pre/lib/pdk/cli/console.rb:75:in `gem_in_bundle_lockfile?'
	from /opt/puppetlabs/pdk/private/ruby/2.4.5/lib/ruby/gems/2.4.0/gems/pdk-1.14.0.pre/lib/pdk/cli/console.rb:125:in `run_in_module'
	from /opt/puppetlabs/pdk/private/ruby/2.4.5/lib/ruby/gems/2.4.0/gems/pdk-1.14.0.pre/lib/pdk/cli/console.rb:45:in `block (2 levels) in <module:CLI>'
	from /opt/puppetlabs/pdk/private/ruby/2.4.5/lib/ruby/gems/2.4.0/gems/cri-2.10.1/lib/cri/command.rb:329:in `run_this'
	from /opt/puppetlabs/pdk/private/ruby/2.4.5/lib/ruby/gems/2.4.0/gems/cri-2.10.1/lib/cri/command.rb:269:in `run'
	from /opt/puppetlabs/pdk/private/ruby/2.4.5/lib/ruby/gems/2.4.0/gems/cri-2.10.1/lib/cri/command.rb:287:in `run'
	from /opt/puppetlabs/pdk/private/ruby/2.4.5/lib/ruby/gems/2.4.0/gems/pdk-1.14.0.pre/lib/pdk/cli.rb:56:in `run'
	from /opt/puppetlabs/pdk/private/ruby/2.4.5/lib/ruby/gems/2.4.0/gems/pdk-1.14.0.pre/exe/pdk:6:in `<top (required)>'
	from /opt/puppetlabs/pdk/private/ruby/2.4.5/bin/pdk:23:in `load'
	from /opt/puppetlabs/pdk/private/ruby/2.4.5/bin/pdk:23:in `<main>'
@logicminds logicminds added bug needs-triage Newly created issue that has not been reviewed by a PDK contributor labels Oct 8, 2019
@logicminds logicminds changed the title in_module_root? gives false negative in_module_root? gives false positive Oct 8, 2019
@rodjek
Copy link
Contributor

rodjek commented Oct 8, 2019

PDK::Util.in_module_root? is deliberately pretty tolerant as it needs to try to determine if you're inside an unconverted module. I would suggest adding PDK::CLI::Util.module_version_check after PDK::CLI::Util.ensure_in_module! which will perform a further validation based on the module metadata and in this case cause the command to exit early saying that the module is not PDK compatible due to the missing metadata.json.

@glennsarti
Copy link
Contributor

Yup, this tripped me up before 😢

@logicminds
Copy link
Contributor Author

Wouldn't lib/puppet or lib/facter be a better directory to search for?

@glennsarti
Copy link
Contributor

Good idea.

@glennsarti glennsarti self-assigned this Oct 21, 2019
@glennsarti glennsarti added this to the November 2019 milestone Oct 21, 2019
@rodjek
Copy link
Contributor

rodjek commented Oct 21, 2019

👍 to making the directory check more specific

glennsarti added a commit to glennsarti/pdk that referenced this issue Oct 21, 2019
@glennsarti
(puppetlabsGH-768) Fix in_module_root? gives false positives
0b985d2 
Previously the module root detection used the bare `lib` directory to detect if
a user was in a module root, without a metadata.json.  However this caused
false positives as `lib` is a very common directory. For example when in a ruby
gem project. This commit modifies the detect to use more puppet specific paths:
* lib/puppet
* lib/facter
* lib/puppet_x

This commit also adds tests for these folder directories, and uses the PDK
Filesystem abstraction classes.
glennsarti added a commit to glennsarti/pdk that referenced this issue Oct 21, 2019
@glennsarti
(puppetlabsGH-768) Mock module root in tests
59adfda 
Previously the module root detection was changed to not detect the bare `lib`
directory however this caused many of the unit tests to fail as it was detecting
the PDK gem as a valid module (which it is not).  This commit adds an empty
module root fixture and mocks the various tests use this as the module root.
glennsarti added a commit to glennsarti/pdk that referenced this issue Oct 21, 2019
@glennsarti
(puppetlabsGH-768) Remove redundant tests
5302c70 
Previously the `ensure_in_module!` tests tested for the various module
directories (lib, tasks and so on).  However this is really just duplicating
the tests for `in_module_root?` which is not required.  This commit removes
these tests and just mocks the response from `in_module_root?`.  This results
in no loss of test coverage.
@glennsarti glennsarti mentioned this issue Oct 21, 2019
Merged
glennsarti added a commit that referenced this issue Oct 21, 2019
@glennsarti
Merge pull request #783 from glennsarti/gh-768-module-root
25ad912 
(GH-768) Fix in_module_root? gives false positives
@rodjek rodjek removed the needs-triage Newly created issue that has not been reviewed by a PDK contributor label Nov 1, 2019
@rodjek rodjek changed the title in_module_root? gives false positive PDK::Util.in_module_root? can give a false positive result Nov 1, 2019
logicminds pushed a commit to nwops/pdk that referenced this issue Nov 30, 2020
@glennsarti @logicminds
(puppetlabsGH-768) Fix in_module_root? gives false positives
36e4d27 
Previously the module root detection used the bare `lib` directory to detect if
a user was in a module root, without a metadata.json.  However this caused
false positives as `lib` is a very common directory. For example when in a ruby
gem project. This commit modifies the detect to use more puppet specific paths:
* lib/puppet
* lib/facter
* lib/puppet_x

This commit also adds tests for these folder directories, and uses the PDK
Filesystem abstraction classes.
logicminds pushed a commit to nwops/pdk that referenced this issue Nov 30, 2020
@glennsarti @logicminds
(puppetlabsGH-768) Mock module root in tests
775d607 
Previously the module root detection was changed to not detect the bare `lib`
directory however this caused many of the unit tests to fail as it was detecting
the PDK gem as a valid module (which it is not).  This commit adds an empty
module root fixture and mocks the various tests use this as the module root.
logicminds pushed a commit to nwops/pdk that referenced this issue Nov 30, 2020
@glennsarti @logicminds
(puppetlabsGH-768) Remove redundant tests
da7a72b 
Previously the `ensure_in_module!` tests tested for the various module
directories (lib, tasks and so on).  However this is really just duplicating
the tests for `in_module_root?` which is not required.  This commit removes
these tests and just mocks the response from `in_module_root?`.  This results
in no loss of test coverage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@glennsarti glennsarti

Labels
bug
Projects
None yet
Milestone
November 2019
Development

Successfully merging a pull request may close this issue.

(GH-768) Fix in_module_root? gives false positives glennsarti/pdk
3 participants
@rodjek @logicminds @glennsarti