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

Nitish add force parameter to create new section #286

Conversation

hsitin
Copy link

@hsitin hsitin commented Jun 4, 2018

No description provided.

Nitish Janawadkar added 2 commits May 23, 2018 22:35
1) Added the has_section to the ini_file class in util folder
2) Converted to nested if
3) Removed reduntant paranthesis around method calls
Copy link
Contributor

@hunner hunner left a comment

Choose a reason for hiding this comment

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

  • Could you describe the purpose of this new parameter in your commit message or PR description.
  • Rubocop is failing on a few things. You can find these in the GitHub CI output
  • The new parameter will need a readme entry before merging.

@@ -39,6 +39,11 @@ def munge_boolean_md5(value)
end
end

newparam(:force_new_section_creation, :boolean => true, :parent => Puppet::Parameter::Boolean) do
desc 'Create setting only if the section exists'
defaultto(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Defaulting to true seems like the opposite behavior of what currently exists?

Copy link
Author

Choose a reason for hiding this comment

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

The current behavior is that it creates the section and the setting if it does not exist in the ini file. So defaulting force_new_section_creation to True will also create the new section.

resource[:ensure] = :absent
resource[:force_new_section_creation]
else
!resource[:force_new_section_creation]
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be idempotent? It seems like it would be trying to call create every puppet run.

Copy link
Author

Choose a reason for hiding this comment

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

I tried different combinations of setting and it does not call create on every puppet run

Nitish Janawadkar added 3 commits June 5, 2018 12:33
1) White spaces were removed
2) renamed functions that return boolean value to appropriate name
@hsitin
Copy link
Author

hsitin commented Jun 6, 2018

  1. Could you describe the purpose of this new parameter in your commit message or PR description.

The intent here is to manage the multiple redhat.repo config files on different servers. The files cannot be managed by Puppet as these will be generated by Satellite dynamically. The current behavior of the module creates new section and setting if it does not exist in the .ini file. What we want to achieve is that the module should not create new section in the .ini file if it does not exist. So we created a new param force_new_section_creation which is set to True by default, so that it doesn't break the previous logic. If it is set to False then it will not create the new section in the .ini file even if it is set to present.

  1. Rubocop is failing on a few things. You can find these in the GitHub CI output
    I have modified the code and fixed the errors. All checks have passed now.

  2. The new parameter will need a readme entry before merging.
    Added to the Readme file

@pmcmaw
Copy link
Contributor

pmcmaw commented Jun 15, 2018

@hunner are you happy enough with the changes?

@david22swan
Copy link
Member

@hunner ???

@davidmalloncares
Copy link
Contributor

merging, @hsitin - you might want to add some tests around the new behaviour though :)

@davidmalloncares davidmalloncares merged commit 4373d19 into puppetlabs:master Jun 22, 2018
@mwhahaha
Copy link
Contributor

@davidmalloncares @hsitin we're seeing failures with this change,

2018-06-22 14:59:50 | 2018-06-22 14:59:50,545 INFO: �[1;31mError: /Stage[main]/Glance::Backend::Swift/Glance_swift_config[ref1/user]: Could not evaluate: Invalid parameter force_new_section_creation(:force_new_section_creation)�[0m

Still investigating

@davidmalloncares
Copy link
Contributor

@mwhahaha np - let me know and we can pull the change back if required. @hsitin can you take a look to see if this has uncovered an issue?

apevec pushed a commit to redhat-openstack/rdoinfo that referenced this pull request Jun 22, 2018
[1] is breaking TripleO deployments, see
https://bugs.launchpad.net/tripleo/+bug/1778247

[2] - puppetlabs/puppetlabs-inifile#286

Change-Id: I241fb6245ee31aff71c1fb405c883f151d8c3dcf
!ini_file.get_value(section, setting).nil?
if ini_file.section?(section)
!ini_file.get_value(section, setting).nil?
elsif !resource[:force_new_section_creation]
Copy link
Contributor

@mwhahaha mwhahaha Jun 22, 2018

Choose a reason for hiding this comment

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

So the issue i'm seeing is with this. If you're child object doesn't have this parameter (none of ours do because we're not inheriting from the upstream ini_setting type) this fails. I think what we need to do is check that this option is a value on the type before trying to consume it.

@mwhahaha
Copy link
Contributor

A proposed fix is available at #288 @hsitin and @davidmalloncares please review

@pmcmaw pmcmaw added the feature label Jun 25, 2018
cegeka-jenkins pushed a commit to cegeka/puppet-inifile that referenced this pull request Aug 5, 2021
…ter_to_create_new_section

Nitish add force parameter to create new section
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.

6 participants