(PDK-1108) Add pdk set config command #859

glennsarti · 2020-02-24T08:20:25Z

Previously the PDK::Config class could only read (get) values, it could not set
them easily, and lacked logic to deal with different value types, for example
if you set a deep hash value, then the save data method on the namespace was
not triggered.

This commit:

Adds a PDK::Config.set method which takes into account nested namespaces,
setting deep hashes, creates missing hash structures, and can add to, or
explicitly set array, values. All of this is necessary for the CLI component
Adds tests for the different scenarios

Now that the PDK::Config class now has a set method, we can add a set config
CLI command so users can set configuration values. Later commits will add the
remove config CLI command.

This commit:

Adds the basic pdk set config command
Adds type conversion using the --type parameter
Adds tests for the CLI command

Previously many calls within the PDK used .user directly. However now that
there is a formal get and set method, accessing these directly is no longer
required and is a private API. This commit updates any references to these
private methods and changes them to use .get and .set appropriately.

TODO

Add PDK::Config.set
Add pdk set config command
Add tests (unit/acceptance)
Replace any internal PDK calls that set config to use the new set method e.g. PDK.config.user['foo']['bar'] = 'baz' to PDK.config.set('foo.bar', 'baz')

coveralls · 2020-02-24T08:32:25Z

Coverage increased (+0.02%) to 91.29% when pulling d9458b1 on glennsarti:finish-pdk-config into 807b1b7 on puppetlabs:master.

rodjek · 2020-02-28T01:02:50Z

spec/acceptance/set_spec.rb

+      # Should show the command help
+      its(:stdout) { is_expected.to match(%r{pdk set \[subcommand\] \[options\]}) }
+      its(:stderr) { is_expected.to have_no_output }
+    end


It'd be good to get some functional tests here as well

The unit tests cover A LOT of those use cases. So it felt strange to duplicate them.

Anything in particular you wanted?

Just a few cases to ensure the expected changes on disk which unit tests mock out. Set at system level, user level, project level, ensure the file gets created on disk at the appropriate path. Seems redundant but it covers cases like #862

Actually, this will be VERY tricky to do in acceptance. I'd either:

Have to mock File.write calls to the expected places on disk

OR

Make the config classes write to a temp file location

Point 1 is already done in spec/unit/pdk/util_spec.rb once #862 is merged

Point 2 is what's being used in this test suite already except only for user files. If I was to use temporary system and project locations then it still wouldn't have picked up the issue #862

If I don't mock, then I'd be overwriting actual file content which is not expected when running acceptance. If I want to write to actual disk locations, this would be appropriate in the package-testing which is using VMs/Containters, not the localhost.

So yeah, that kind of testing needs to go in package-testing.

@rodjek I've added a new commit which adds some package-testing tests.

glennsarti · 2020-02-28T01:20:16Z

Putting back to WIP until the acceptance tests are updated.

lib/pdk/cli/set/config.rb

scotje · 2020-02-28T01:23:13Z

spec/acceptance/set_config_spec.rb

+        its(:stdout) { is_expected.to match(%r{user.module_defaults.mock=value}) }
+        its(:stderr) { is_expected.to match(%r{Set initial value of 'user.module_defaults.mock' to 'value'}) }
+
+        it_behaves_like 'a saved configuration file', "{\n  \"mock\": \"value\"\n}\n"


Is it worth having a JSON-specific shared behavior that parses the file contents so we don't have to serialize the whitespace and everything?

E.g.

it_behaves_like 'a saved JSON configuration file', { "mock" => "value" }

Changed the bulk of the tests to use JSON (ala Hash) matching and just one for file content. I need that one to prove it's outputting "pretty" JSON instead of mini-fied JSON which is hard to read.

spec/acceptance/set_config_spec.rb

spec/unit/pdk/cli/set/config_spec.rb

Previously the PDK::Config class could only read (get) values, it could not set them easily, and lacked logic to deal with different value types, for example if you set a deep hash value, then the save data method on the namespace was not triggered. This commit: * Adds a PDK::Config.set method which takes into account nested namespaces, setting deep hashes, creates missing hash structures, and can add to, or explicitly set array, values. All of this is necessary for the CLI component * Adds tests for the different scenarios

Now that the PDK::Config class now has a set method, we can add a `set config` CLI command so users can set configuration values. Later commits will add the `remove config` CLI command. This commit: * Adds the basic `pdk set config` command * Adds type conversion using the `--type` parameter * Adds tests for the CLI command

Previously there were no full tests for the get and set configuration. This commit adds package-testing tests which modifies configuration files on actual systems during testing.

Previously many calls within the PDK used `.user` directly. However now that there is a formal get and set method, accessing these directly is no longer required and is a private API. This commit updates any references to these private methods and changes them to use .get and .set appropriately.

spec/unit/pdk/cli/set/config_spec.rb

This commit updates the tests to use the anything matchcer instead of Object as per recommended practice in rspec-mocks.

glennsarti added the WIP label Feb 24, 2020

glennsarti requested a review from a team as a code owner February 24, 2020 08:20

glennsarti force-pushed the finish-pdk-config branch from b6ff21d to 5b8962d Compare February 24, 2020 08:59

glennsarti removed the WIP label Feb 24, 2020

rodjek reviewed Feb 28, 2020

View reviewed changes

glennsarti added the WIP label Feb 28, 2020

scotje reviewed Feb 28, 2020

View reviewed changes

glennsarti force-pushed the finish-pdk-config branch from 5b8962d to f67c801 Compare February 28, 2020 03:53

glennsarti force-pushed the finish-pdk-config branch from f67c801 to 3536bea Compare February 28, 2020 06:24

glennsarti removed the WIP label Feb 28, 2020

glennsarti mentioned this pull request Feb 28, 2020

(FIXUP) Make system config path absolute instead of relative on POSIX hosts #862

Merged

scotje added this to the March 2020 milestone Mar 3, 2020

glennsarti added 2 commits March 3, 2020 13:06

(PDK-1108) Add package tests for PDK get and set config

077da68

Previously there were no full tests for the get and set configuration. This commit adds package-testing tests which modifies configuration files on actual systems during testing.

glennsarti force-pushed the finish-pdk-config branch from 3536bea to 9364cde Compare March 3, 2020 05:06

rodjek reviewed Mar 4, 2020

View reviewed changes

spec/unit/pdk/cli/set/config_spec.rb Outdated Show resolved Hide resolved

(PDK-1108) Use idiomatic rspec-mocks

d9458b1

This commit updates the tests to use the anything matchcer instead of Object as per recommended practice in rspec-mocks.

rodjek approved these changes Mar 4, 2020

View reviewed changes

glennsarti merged commit c401002 into puppetlabs:master Mar 5, 2020

glennsarti deleted the finish-pdk-config branch March 5, 2020 04:56

glennsarti modified the milestones: March 2020, April 2020 Mar 27, 2020

scotje added the feature label May 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PDK-1108) Add pdk set config command #859

(PDK-1108) Add pdk set config command #859

glennsarti commented Feb 24, 2020 •

edited

Loading

coveralls commented Feb 24, 2020 •

edited

Loading

rodjek Feb 28, 2020

glennsarti Feb 28, 2020 •

edited

Loading

rodjek Feb 28, 2020

glennsarti Feb 28, 2020

glennsarti Feb 28, 2020

glennsarti Feb 28, 2020

glennsarti commented Feb 28, 2020

scotje Feb 28, 2020

glennsarti Feb 28, 2020

(PDK-1108) Add pdk set config command #859

(PDK-1108) Add pdk set config command #859

Conversation

glennsarti commented Feb 24, 2020 • edited Loading

coveralls commented Feb 24, 2020 • edited Loading

rodjek Feb 28, 2020

Choose a reason for hiding this comment

glennsarti Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

rodjek Feb 28, 2020

Choose a reason for hiding this comment

glennsarti Feb 28, 2020

Choose a reason for hiding this comment

glennsarti Feb 28, 2020

Choose a reason for hiding this comment

glennsarti Feb 28, 2020

Choose a reason for hiding this comment

glennsarti commented Feb 28, 2020

scotje Feb 28, 2020

Choose a reason for hiding this comment

glennsarti Feb 28, 2020

Choose a reason for hiding this comment

glennsarti commented Feb 24, 2020 •

edited

Loading

coveralls commented Feb 24, 2020 •

edited

Loading

glennsarti Feb 28, 2020 •

edited

Loading