-
Notifications
You must be signed in to change notification settings - Fork 104
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-1107) Add pdk config get CLI command #715
(PDK-1107) Add pdk config get CLI command #715
Conversation
Looked at adding json formatter but it's waaay outside the scope of this PR and should not stop merging of this PR. Punting for now |
b59226b
to
c8ee105
Compare
Not sure about this though: https://github.com/puppetlabs/pdk/pull/715/files#diff-583f8159ac861abd5736f8e9c611c396R35 If the user specifies a non-existant setting, should it exit with non-zero? Update : Yes it should be non-zero Update : Now exits with 1 |
4bc2cd6
to
f613c8c
Compare
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.
This is looking good so far!
I do notice a couple differences in the base output format from the RFC in this implementation*, but I'm not sure if the plan is to refine that in future PRs. I'm definitely +1 to implementing things incrementally.
* For example, requesting the value of a leaf node:
RFC:
If
<key>
is a leaf node of the configuration graph, this command will return the raw value
$ pdk config get user.analytics.user-id
4489233c-e1ff-4761-9ee0-5c85b713da67
Implementation:
$ pdk config get user.analytics.user-id
user.analytics.user-id=4489233c-e1ff-4761-9ee0-5c85b713da67
I'm also fine with revising what is described in the RFC if we want to do that deliberately.
As there is no formal spec on what settings should appear where (i.e. what's a leaf vs, what's a tree) it get's tricky. So instead I guess. If user specified |
f613c8c
to
123c6f0
Compare
@scotje I've ticketted changes to the RFC to match this PR at https://github.com/puppetlabs/pdk-planning/issues/40 Once that is issue is approved and merged, this PR can be merged. |
Yeah, we had talked about having a config schema we could validate against to make this easier to determine as well. This section of the RFC was a initial draft of allowed keys and values at the various layers: https://github.com/puppetlabs/pdk-planning/blob/master/RFCs/0002-add-pdk-config.md#proposed-configuration-keys but that will certainly be expanded/revised as we go along. I do think we should do some sort of checking against a schema to make sure people don't typo a setting key and then wonder why it's not working. |
I did see that. Probably best to introduce a schema validator now actually |
Back to WIP for you! |
Blocked on #726 merge |
RFC update has been merged |
123c6f0
to
9918103
Compare
@scotje Regarding While I don't have a schema to work with, the tests show the current logic exhibits the behaviour the RFC sets out. |
9918103
to
95fc1a1
Compare
Previously there was no way to display what the current configuration the PDK was using. This commit adds a `pdk config` command, with a single `get` sub-command. This commit also adds the ability to filter the settings either by a specific name or setting treename. This commit also adds tests for the new resolve method in the Namespace object.
95fc1a1
to
5cb9b92
Compare
CI Is green and approvals done. Merging. |
Needs to format as json?punting. too hard and not neededAdd validation and schemaSeparate PR {WIP}(PDK-1112) Use schema documents for configuration settings #728Previously there was no way to display what the current configuration the PDK
was using. This commit adds a
pdk config
command, with a singleget
sub-command.
This commit also adds the ability to filter the settings either by a specific
name or setting treename.
This commit also adds tests for the new resolve method in the Namespace object.