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

(PDK-1607)(PDK-1608) Implement system-level settings for PDK configuration #841

Merged

Conversation

glennsarti
Copy link
Contributor

@glennsarti glennsarti commented Feb 3, 2020

Blocked by #849


PDK-1607

Previously in commit 5cb9b92 the pdk config get command was added however
this was in contrary to the Verb-Noun UX for the PDK CLI. This commit:

  • Copies the pdk config and pdk config get code (and tests) to be
    pdk get and pdk get config respectively
  • Adds deprecation notices to the pdk config * commands
  • Updates tests for the deprecation notices

PDK-1608

Previously only per-user settings existed for the PDK::Config classes. This
commit:

  • Adds the concept and file locations for system level settings
  • Adds helper methods to PDK::Config to make it easier to query multilevel
    settings, and to query using precedence
  • Makes it easier to eventually add per-project settings
  • Adds tests for the new methods
  • Deprecates the use of ".user". Once a ".set(..)" method is completed in the
    future, this method will be removed entirely

Note that the various PDK Modules and Classes still won't consume system-level
settings until they use the PDK.config.pdk_setting(... method. Currently they
still use PDK.config.user[.... This will changed in later commits.

@glennsarti glennsarti requested a review from a team as a code owner February 3, 2020 04:14
@glennsarti glennsarti self-assigned this Feb 3, 2020
@coveralls
Copy link

coveralls commented Feb 3, 2020

Coverage Status

Coverage decreased (-0.2%) to 91.213% when pulling 7a32baa on glennsarti:pdk-1608-system-level-settings into bfcd14a on puppetlabs:master.

lib/pdk/cli/config.rb Outdated Show resolved Hide resolved
lib/pdk/cli/config/get.rb Outdated Show resolved Hide resolved
lib/pdk/config.rb Outdated Show resolved Hide resolved
Comment on lines 55 to 71
def get(*name)
return nil if name.nil? || !name.is_a?(Array) || name.empty?
case name[0]
when 'user'
traverse_object(user, *name[1..-1])
when 'system'
traverse_object(self.system, *name[1..-1]) # rubocop:disable Style/RedundantSelf Use an explicit call so as to not confuse with Kernel.system
else
nil
end
end

# Gets a named setting using precedence from the user and system levels
# Note that name must NOT include user or system prefix
def pdk_setting(*name)
value = get(*['user'].concat(name))
value.nil? ? get(*['system'].concat(name)) : value
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we are mostly going to want the "lookup using precedence" behavior that is currently implemented in pdk_setting(), so maybe what is currently get() should be like get_within_scope(scope, *name) and then the "lookup using precedence" can just become get()?

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 disagree here, but I believe I can the point your trying to make here. And I guess it depends on the consumers point of view.

In the future, we'll have at least one addition setting level, project., or even multiple different ones (control_repo. or module.).

So I see

PDK::Config.get(...) to be the most specific form. "Get me this setting name"

PDK::Config.pdk_setting to be a helper, to stop copying the precedence logic. I chose pdk_setting so it would (hopefully) be obvious that this is only a PDK setting, not a Project setting.

I do concede that PDK::Config.pdk_setting seems like a redundant name.

As for your suggestion:

get_within_scope(scope, *name) makes perfect sense

but get() only returning user and system level settings may be a confusing method name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, I do like that we both agree on the layout of the methods, just the method names need some discussion 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scotje You happy for merge are would prefer a name change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't it seem possible people will want to override a "PDK" setting at the project scope at some point as well? Or is it clear that the system/user settings will never overlap with project settings?

It seems like what we need for a general interface is:

  • get a setting based on default precedence rules (I think that would be project > user > system?)
  • get a setting from a specific precedence level
  • (maybe?) get a setting from a specific subset of precedence levels (for when the caller has a special reason for bypassing normal precedence)

So maybe it's just that get_within_scope should also accept a list for the scope parameter? Or we could have a get_within_scopes?

Copy link
Contributor Author

@glennsarti glennsarti Feb 17, 2020

Choose a reason for hiding this comment

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

Okay, I've basically implemented what you stated. I've gone with:

  • .get This gets a fully named setting e.g. user.foo.bar. No scope precedence, just literal "Get this setting called <blah>"

  • .get_within_scopes : This gets a setting name in zero or more specified scopes (in order). If no scopes are given (nil) then it searches in the default scopes in the default order.

Which ends up with:

  • For the bulk of calls within /lib/cli/get/config .get is probably the best
  • For the bulk of calls within /lib/* .get_within_scopes is probably the best
  • For critical calls like analytics information, it can be restricted to .get_within_scopes('....', ['user', 'system']). Later on, I may add a shortcut method (def critical_scopes; ['user', 'system'].freeze; end;) if I find that we use it too often.

@glennsarti glennsarti force-pushed the pdk-1608-system-level-settings branch 2 times, most recently from 6e63e50 to 15078ec Compare February 7, 2020 06:55
@glennsarti
Copy link
Contributor Author

The Travis failure is incidental to this PR.

Previously in commit 5cb9b92 the `pdk config get` command was added however
this was in contrary to the Verb-Noun UX for the PDK CLI.  This commit:

* Copies the `pdk config` and `pdk config get` code (and tests) to be
  `pdk get` and `pdk get config` respectively
* Adds deprecation notices to the `pdk config *` commands
* Updates tests for the deprecation notices
@glennsarti glennsarti force-pushed the pdk-1608-system-level-settings branch 2 times, most recently from 3acd956 to 2ed9624 Compare February 17, 2020 02:32
Previously only per-user settings existed for the PDK::Config classes.  This
commit:
* Adds the concept and file locations for system level settings
* Adds helper methods to PDK::Config to make it easier to query multilevel
  settings, and to query using precedence
* Makes it easier to eventually add per-project settings
* Adds tests for the new methods
* Deprecates the use of ".user". Once a ".set(..)" method is completed in the
  future, this method will be removed entirely

Note that the various PDK Modules and Classes still won't consume system-level
settings until they use the `PDK.config.pdk_setting(...` method.  Currently they
still use `PDK.config.user[...`.  This will changed in later commits.
Copy link
Contributor

@scotje scotje left a comment

Choose a reason for hiding this comment

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

Just a couple questions about .dup but overall this is looking great!

lib/pdk/config/namespace.rb Outdated Show resolved Hide resolved
lib/pdk/config/setting.rb Outdated Show resolved Hide resolved
Previously it was possible to get a current PDK configuration setting and then
accidentally make changes to it, which would bypass the data saving code.  This
would also corrupt the setting when other parts of the PDK try to access the
same setting.

This commit duplicates the setting and default values which now isolates the
value from any changes.  And adds tests for the isolation.
Copy link
Contributor

@scotje scotje left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@glennsarti glennsarti merged commit 64a6d43 into puppetlabs:master Feb 20, 2020
@rodjek rodjek added the feature label Feb 26, 2020
@glennsarti glennsarti deleted the pdk-1608-system-level-settings branch March 5, 2020 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants