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

refs #12179 - refactoring extracted strings for host parameters #2948

Closed
wants to merge 1 commit into from

Conversation

orrabin
Copy link
Member

@orrabin orrabin commented Dec 1, 2015

@domcleal I changed host_params.rb as you requested and fixed the rest of the rest of the parameters shown on host edit form to be consistent.
This might be overkill, in which case I'll close the pr.

@@ -165,7 +165,7 @@ def parent_params(include_source = false)
# otherwise we might be overwriting the hash in the wrong order.
groups = Hostgroup.sort_by_ancestry(Hostgroup.includes(:group_parameters).find(ids))
groups.each do |hg|
hg.group_parameters.each {|p| hash[p.name] = include_source ? {:value => p.value, :source => N_('hostgroup').to_sym, :safe_value => p.safe_value, :source_name => hg.title} : p.value }
hg.group_parameters.each {|p| hash[p.name] = include_source ? {:value => p.value, :source => p.associated_type.to_sym, :safe_value => p.safe_value, :source_name => hg.title} : p.value }
Copy link
Member

Choose a reason for hiding this comment

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

Why does :source here and elsewhere call .to_sym?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's any good reason for this.

@orrabin
Copy link
Member Author

orrabin commented Dec 1, 2015

test failures are unrelated

@@ -160,6 +160,12 @@ def get_element_and_element_name(lookup_value)
element_name = ''
lookup_value.match.split(LookupKey::KEY_DELM).each do |match_key|
lv_element, lv_element_name = match_key.split(LookupKey::EQ_DELM)
case lv_element
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little unsure about this because this is actually the matcher defined by the user in the Puppet class settings. To change it for some objects and not others might be confusing, and open up another can of worms.

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'm fine with not changing this but that means you would look at the additional information for smart class parameters and see 'hostgroup' and in global parameters 'host group'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I understand. I don't mind if you want to address it (separately?), but I'd have more comments - since we'd need to translate everything, not just these two resource types.

@orrabin
Copy link
Member Author

orrabin commented Dec 2, 2015

@domcleal I fixed all of your comments and also removed the to_sym as @tbrisker suggested.

@domcleal
Copy link
Contributor

domcleal commented Dec 7, 2015

Merged as 18c4b17, thanks @orrabin.

@domcleal domcleal closed this Dec 7, 2015
stbenjam pushed a commit to stbenjam/foreman that referenced this pull request Feb 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants