Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow but filter out empty strings in build_dn #460

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Jan 27, 2023

There exists the possibility a parameter passed to build_dn could be an empty string which breaks the previous requirement on a string of length at least one, String[1]. Change to accept any length string but if the string happens to be empty, filter it out of the calculation.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I'm fine either way, let me know if you think my suggestion may be better.

return_type 'String'
end

def build_dn(options)
options.select { |_key, value| value }.map { |key, value| "#{key}=#{value}" }.join(', ')
options_with_values = options.filter { |_key, value| !value.nil? && value != '' }
Copy link
Member

Choose a reason for hiding this comment

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

I think this is more expressive in what it tries to achieve:

Suggested change
options_with_values = options.filter { |_key, value| !value.nil? && value != '' }
options_with_values = options.delete_if { |_key, value| value.nil? || value == '' }

I admit Ruby is always a bit weird since there's multiple ways of doing things. For example filter is an alias for select.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think if we use delete_if it becomes:

  options.delete_if { |_key, value| value.nil? || value == '' }

My understanding reading the docs on it is that it's an in place modifier.

Copy link
Member Author

@ehelms ehelms Feb 1, 2023

Choose a reason for hiding this comment

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

And testing, this actually leads to an error:

        expected katello::build_dn([["a", nil], ["b", "2"]]) to have returned "b=2" instead of raising FrozenError(can't modify frozen Array: [["a", nil], ["b", "2"]])
        /home/ehelms/workspace/upstream/installer/puppet-katello/spec/fixtures/modules/katello/lib/puppet/functions/katello/build_dn.rb:17:in `delete_if'

So based on that I'll keep it as is.

Copy link
Member

@ekohl ekohl Feb 1, 2023

Choose a reason for hiding this comment

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

That makes sense, thanks.

Though I think you should use select instead of filter for Ruby 2.5 support:

  Evaluation Error: Error while evaluating a Function Call, undefined method `filter' for #<Array:0x0000561f726d9560> (file: /home/runner/work/puppet-katello/puppet-katello/spec/fixtures/modules/katello/manifests/application.pp, line: 40, column: 24) on node fv-az572-767.xfljd2mj00zetb5gvhv1u2zrtd.dx.internal.cloudapp.net

@ehelms ehelms force-pushed the allow-build-dn-empty branch 2 times, most recently from c815b5e to ca841ac Compare February 1, 2023 14:36
There exists the possibility a parameter passed to build_dn could be
an empty string which breaks the previous requirement on a string
of length at least one, String[1]. Change to accept any length string
but if the string happens to be empty, filter it out of the calculation.
@ehelms ehelms force-pushed the allow-build-dn-empty branch from ca841ac to 0a77ef5 Compare February 1, 2023 14:59
@ekohl ekohl enabled auto-merge (rebase) February 1, 2023 15:00
@ekohl ekohl merged commit 5765a62 into theforeman:master Feb 1, 2023
@ehelms ehelms added the Bug label Feb 2, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants