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

Add backslash support to registry resource #124

Closed
wants to merge 7 commits into from

Conversation

sbrito85
Copy link

@sbrito85 sbrito85 commented Feb 8, 2017

Puppet bug here: https://tickets.puppetlabs.com/browse/MODULES-2957

Picked up the work being done on other pull requests.

Added a new value_name property to handle more complex values
Kept backwards compatibility by handling empty values and creating
'value_name' using the last element in the key path.
@sbrito85
Copy link
Author

sbrito85 commented Feb 8, 2017

Looks like I need to clean up some tests :)

@jpogran
Copy link
Contributor

jpogran commented Feb 19, 2017

@sbrito85 thanks for the PR! I've added this to triage and it will be reviewed this week. While that happens, could you review the CONTRIBUTING.md file and update your commits? Specifically referring to commit message section.

Also, we will need to add a spec test for this. If you don't have time to do so, I will add it before the PR is merged

@@ -103,7 +103,7 @@ def eval_generate

# get the "should" names of registry values associated with this key
should_values = catalog.relationship_graph.direct_dependents_of(self).select {|dep| dep.type == :registry_value }.map do |reg|
PuppetX::Puppetlabs::Registry::RegistryValuePath.new(reg.parameter(:path).value).valuename
PuppetX::Puppetlabs::Registry::RegistryValuePath.new(reg.parameter(:path).value, reg.parameter(:value_name).value).valuename
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean it's a breaking change? As in, do users using the old syntax have to use this new syntax?

Copy link
Author

Choose a reason for hiding this comment

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

Old syntax still works from all tests that I've done. This should not be a breaking change.

@glennsarti
Copy link
Contributor

Hi @sbrito85. Thanks for all your work on this so far. I have some concerns about it;

  • Needs documentation update to explain how to use a value with a name with '`
  • Needs spec tests for valuenames with a \
  • I briefly started adding some tests and noticed there are some issues with the auto-require logic as it doesn't check for resources with value_name

@sbrito85
Copy link
Author

I plan on having some cycles this week to look at these concerns. Sorry for the delay!

  1. Will add documentation, should have that in today or tomorrow
  2. For this part, I may need some assistance.
  3. Where should I be checking for this? I presume it should go in lib/puppet/type/registry_value.rb like the path value?

@glennsarti
Copy link
Contributor

@sbrito85

I started work on squashing your commits, and then adding tests.

Commits are here;
https://github.com/glennsarti/puppetlabs-registry/commits/ticket/master/MODULES-2957-backslash

This is what tripped me up with autorequire as those tests were failing.

The autorequire code is here
https://github.com/puppetlabs/puppetlabs-registry/blob/master/lib/puppet/type/registry_value.rb#L124-L137

Note it's using the resource name not the non-backslash value.

@michaeljmatthews22
Copy link

Any update on this? Really looking forward to this being available for our company.

@makomatic
Copy link

Any Update? This would reallly be nice...

@Iristyle
Copy link
Contributor

This is being superseded in #143 and #142 - as such, closing.

Thanks for the original PR @sbrito85!

@Iristyle Iristyle closed this Dec 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants