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

Finish API conversion of create_ini_settings #387

Merged

Conversation

alexjfisher
Copy link
Contributor

In #373 the
create_ini_settings function was ported to the new function API and
namespaced. The boilerplate wasn't fixed up and the function didn't even
work...

This commit finishes the conversion with the following changes:

  • Remove the old API puppet/parser/function version.
  • Create a replacement non-namespaced, (but deprecated), new API shim
    that calls the namespaced version.
  • Replace argument validation in the function code with data-type based
    validation for the individual function parameters.
  • Use call_function instead of function_create_resources.
  • Update the README and REFERENCE.md files.
  • Test both shim and namespaced version of the function.

@alexjfisher alexjfisher requested a review from a team as a code owner March 3, 2020 22:13
@@ -188,12 +188,12 @@ resources { 'glance_api_config':

### Manage multiple ini_settings

To manage multiple `ini_settings`, use the [`create_ini_settings`](#function-create_ini_settings) function.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous link was broken.

@@ -243,17 +243,14 @@ ini_setting { '[section1] setting2':

#### Manage multiple ini_settings with Hiera

This example requires Puppet 3.x/4.x, as it uses automatic retrieval of Hiera data for class parameters and `puppetlabs/stdlib`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This statement doesn't seem helpful anymore.

) {
validate_hash($settings)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

validate_hash was why the removed statement above talked about stdlib

@@ -300,8 +297,6 @@ See [REFERENCE.md](https://github.com/puppetlabs/puppetlabs-inifile/blob/master/
<a id="limitations"></a>
## Limitations

Due to (PUP-4709) the create_ini_settings function will cause errors when attempting to create multiple ini_settings in one go when using Puppet 4.0.x or 4.1.x. If needed, the temporary fix for this can be found here: https://github.com/puppetlabs/puppetlabs-inifile/pull/196.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As well as Puppet 5 being the minimum supported version, the 'temporary fix' referenced was merged years ago.

repeated_param 'Any', :args
end
def deprecation_gen(*args)
call_function('deprecation', 'create_ini_settings', 'This method is deprecated, please use inifile::create_ini_settings instead.')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

deprecation comes from stdlib. I look forward to the namespaced version of that function! ;)

@@ -1,49 +1,15 @@
# This is an autogenerated function, ported from the original legacy version.
# It /should work/ as is, but will not have all the benefits of the modern
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should work, but didn't due to the way create_resources was being called.

},
{
"name": "puppetlabs/stdlib",
"version_requirement": ">= 4.13.0 < 7.0.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The deprecation function was added in 4.13.0.

it { is_expected.to run.with_params('section' => { 'setting' => 'value' }).and_raise_error(Puppet::ParseError, %r{must pass the path parameter}) }
it { is_expected.to run.with_params(1 => 2).and_raise_error(Puppet::ParseError, %r{Section 1 must contain a Hash}) }
end
it_behaves_like 'create_ini_settings function'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of repeating the same tests for both the namespaced and non-namespaced versions, I've used a shared example.

In puppetlabs#373 the
`create_ini_settings` function was ported to the new function API and
namespaced. The boilerplate wasn't fixed up and the function didn't even
work...

This commit finishes the conversion with the following changes:

* Remove the old API `puppet/parser/function` version.
* Create a replacement non-namespaced, (but deprecated), new API shim
that calls the namespaced version.
* Replace argument validation in the function code with data-type based
validation for the individual function parameters.
* Use `call_function` instead of `function_create_resources`.
* Update the README and REFERENCE.md files.
* Test both shim and namespaced version of the function.
'ensure' => 'absent'
}
}
}

create_ini_settings($example, $defaults)
inifile::create_ini_settings($example, $defaults)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is updated to use the namespaced version, but I've left spec/fixtures/create_ini_settings_test/manifests/init.pp as-is to use the non-namespaced shim.

Copy link
Contributor

@sanfrancrisko sanfrancrisko left a comment

Choose a reason for hiding this comment

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

Thanks for plumbing in the rest of the functionality @alexjfisher !

@sanfrancrisko
Copy link
Contributor

Release checks pass for this change too.

@sanfrancrisko sanfrancrisko merged commit 9274331 into puppetlabs:master Mar 10, 2020
cegeka-jenkins pushed a commit to cegeka/puppet-inifile that referenced this pull request Aug 5, 2021
…nversion

Finish API conversion of `create_ini_settings`
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.

3 participants