(PDK-1113) Use PDK configuration instead of AnswerFile class #842

glennsarti · 2020-02-04T12:23:00Z

~~Builds on #841~~ Merged

Previously the PDK had two ways to retrieve the same configuration information.
One via the PDK::Config classes and the other the AnswerFile class. This should
only be one. This commit removes the AnswerClass (and associated features) and
instead uses PDK::Config for setting management

Removes the PDK::AnswerFile class, and instead has a single helper method for
the default file location. This may eventually be moved into the PDK::Config
namespace as well
Removes the hidden --answer-file command line option and instead uses an
environment variable PDK_ANSWER_FILE. This feature is not supported and is
only used to acceptance testing. Note that we moved to an environment
variable as there was a subtle race condition where the CLI reads from
PDK::Config, but the CLI also moves the user answer file location. By using
an environment variable this race condition can no longer occur.
Move setting reads from PDK.answers to PDK.config.pdk_setting(....)
Move setting writes from PDK.answers.update! to
PDK.config.user['module_defaults']['....'] =
Modify tests for the setting read/write changes
Added a Mock configuration RSpec Context to stop any test information
affecting actual real setting files. Previously module answer files were
being modified

coveralls · 2020-02-04T12:34:49Z

Coverage increased (+0.009%) to 91.327% when pulling 8955729 on glennsarti:pdk-1113-use-config into 5211ebf on puppetlabs:master.

scotje · 2020-02-06T21:26:39Z

+10 to folding answer file functionality into pdk config.

This does not actually remove the file yet though correct? Just a unified frontend to the existing file alongside other config?

glennsarti · 2020-02-07T03:49:07Z

This does not actually remove the file yet though correct? Just a unified frontend to the existing file alongside other config?

Correct, and I don't think we'll actually need to move the file either. It's doing a perfectly fine job right now, and for the most part hidden to users day-to-day activities.

I'm actually thinking of adding a pdk get config-files command line option to list all of the configuration file locations, for each of the PDK setting roots e.g.

> pdk get config-files
{
   "user": "something/somewhere.json",
   "user.module_defaults": "something/other_dir/somewhere.json",
... and so on
}

scotje · 2020-02-10T19:39:39Z

To me, it makes sense to leave the config files referenced by other tools in place and just provide a frontend to them as you say.

For files only referenced by PDK however, I'm in favor of eventually converging those into a single file for each config layer for simplification.

glennsarti · 2020-02-12T08:02:07Z

~~The rebase has broken lots of stuff. Back to WIP~~ Fixed

Previously the load_data method took a single string as the name of the file to load, however the actual file loaded was the file attribute of the class. This commit modifies the file loading to actually load the filename passed. This hasn't been an issue as yet, as both the filename and file parameters have been the same.

glennsarti · 2020-02-20T01:33:56Z

Need to change this PR for the changes of PR 841

rodjek · 2020-02-20T04:01:08Z

lib/pdk/generate/module.rb


        defaults = PDK::Module::Metadata::DEFAULTS.dup

        defaults['name'] = "#{opts[:username]}-#{opts[:module_name]}" unless opts[:module_name].nil?
-        defaults['author'] = PDK.answers['author'] unless PDK.answers['author'].nil?
-        defaults['license'] = PDK.answers['license'] unless PDK.answers['license'].nil?
+        defaults['author'] = PDK.config.get_within_scopes('module_defaults.author') unless PDK.config.get_within_scopes('module_defaults.author').nil?


We should come up with a helper around this get-and-set-unless-nil pattern that isn't so verbose. Maybe a method that yields the value to a block if not nil?

Something like

PDK.config.with_scoped_value('module_defaults.author') { |v| defaults['author'] = v }

I like that! Will implement.

Added a commit which does this.

spec/acceptance/convert_spec.rb

spec/acceptance/template_ref_spec.rb

Previously the PDK had two ways to retrieve the same configuration information. One via the PDK::Config classes and the other the AnswerFile class. This should only be one. This commit removes the AnswerClass (and associated features) and instead uses PDK::Config for setting management * Removes the PDK::AnswerFile class, and instead has a single helper method for the default file location. This may eventually be moved into the PDK::Config namespace as well * Removes the hidden `--answer-file` command line option and instead uses an environment variable PDK_ANSWER_FILE. This feature is not supported and is only used to acceptance testing. Note that we moved to an environment variable as there was a subtle race condition where the CLI reads from PDK::Config, but the CLI also moves the user answer file location. By using an environment variable this race condition can no longer occur. * Move setting reads from PDK.answers to PDK.config.pdk_setting(....) * Move setting writes from PDK.answers.update! to PDK.config.user['module_defaults']['....'] = * Modify tests for the setting read/write changes * Added a Mock configuration RSpec Context to stop any test information affecting actual real setting files. Previously module answer files were being modified

Previously the user needed to write verbose amounts of code to "do something if and only if the setting value is not nil". This commit adds a helper called `with_scoped_value` which will yield the setting value if and only if the value is not nil. This commit then updates the PDK to use this helper and adds tests for this new helper method.

glennsarti · 2020-02-20T06:41:26Z

CI is green!

glennsarti added the blocked label Feb 4, 2020

glennsarti requested a review from a team as a code owner February 4, 2020 12:23

glennsarti self-assigned this Feb 4, 2020

glennsarti force-pushed the pdk-1113-use-config branch from 26c9b9d to fa90e7a Compare February 12, 2020 06:28

glennsarti added the WIP label Feb 12, 2020

glennsarti force-pushed the pdk-1113-use-config branch 3 times, most recently from 3335918 to 91127fc Compare February 19, 2020 07:05

glennsarti force-pushed the pdk-1113-use-config branch from 91127fc to c13edbc Compare February 20, 2020 01:11

glennsarti removed the blocked label Feb 20, 2020

glennsarti force-pushed the pdk-1113-use-config branch from c13edbc to ae44fda Compare February 20, 2020 02:00

rodjek suggested changes Feb 20, 2020

View reviewed changes

glennsarti added 2 commits February 20, 2020 14:15

glennsarti force-pushed the pdk-1113-use-config branch from ae44fda to 8955729 Compare February 20, 2020 06:15

rodjek approved these changes Feb 21, 2020

View reviewed changes

glennsarti merged commit 41c7fcc into puppetlabs:master Feb 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PDK-1113) Use PDK configuration instead of AnswerFile class #842

(PDK-1113) Use PDK configuration instead of AnswerFile class #842

glennsarti commented Feb 4, 2020 •

edited

Loading

coveralls commented Feb 4, 2020 •

edited

Loading

scotje commented Feb 6, 2020

glennsarti commented Feb 7, 2020

scotje commented Feb 10, 2020

glennsarti commented Feb 12, 2020 •

edited

Loading

glennsarti commented Feb 20, 2020

rodjek Feb 20, 2020

glennsarti Feb 20, 2020

glennsarti Feb 20, 2020

glennsarti commented Feb 20, 2020

(PDK-1113) Use PDK configuration instead of AnswerFile class #842

(PDK-1113) Use PDK configuration instead of AnswerFile class #842

Conversation

glennsarti commented Feb 4, 2020 • edited Loading

coveralls commented Feb 4, 2020 • edited Loading

scotje commented Feb 6, 2020

glennsarti commented Feb 7, 2020

scotje commented Feb 10, 2020

glennsarti commented Feb 12, 2020 • edited Loading

glennsarti commented Feb 20, 2020

rodjek Feb 20, 2020

Choose a reason for hiding this comment

glennsarti Feb 20, 2020

Choose a reason for hiding this comment

glennsarti Feb 20, 2020

Choose a reason for hiding this comment

glennsarti commented Feb 20, 2020

glennsarti commented Feb 4, 2020 •

edited

Loading

coveralls commented Feb 4, 2020 •

edited

Loading

glennsarti commented Feb 12, 2020 •

edited

Loading