-
Notifications
You must be signed in to change notification settings - Fork 145
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
(PDK-1100) Use PDK to build module packages #260
Conversation
Codecov Report
@@ Coverage Diff @@
## master #260 +/- ##
=========================================
- Coverage 40.46% 40.4% -0.07%
=========================================
Files 10 10
Lines 734 750 +16
=========================================
+ Hits 297 303 +6
- Misses 437 447 +10
Continue to review full report at Codecov.
|
CLA signed by all contributors. |
require 'puppet/face' | ||
pmod = Puppet::Face['module', :current] | ||
pmod.build('./') | ||
require 'pdk/module/build' |
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.
Concerned about having this adding pdk gem as a soft dependency. If the default behavior is to use pdk's build feature as a library method, then perhaps we should add pdk as a runtime dependency.
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'm OK with the soft dependency for accessing it as a library but I think there should be a more explicit error message at the end if pdk
is not available on their PATH, or fall back to loading the old face which I think is what Bryan is suggesting below?
path = PDK::Module::Build.invoke(:force => true, :'target-dir' => File.join(Dir.pwd, 'pkg')) | ||
puts "Module built: #{path}" | ||
rescue LoadError | ||
system('pdk build --force') |
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.
Should we also check if Puppet::Face['module', :current]
is nil or returns an error? And potentially default to the old way if the puppet < 6 is being loaded?
Attempt to load the PDK gem first in order to use PDK as a library. If that fails, fall back to executing `pdk build`. *Note* This depends on PDK 1.7.1 being released and so shouldn't be merged until then.
OK, I've gone back to a soft dependency for now until we can resolve the As the logic currently stands:
|
Attempt to load the PDK gem first in order to use PDK as a library. If
that fails, fall back to executing
pdk build
.Note This depends on PDK 1.7.1 being released and so shouldn't be
merged until then.