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

Adjust Puppet Master and Puppet CA wording and tooltip on Host records #7810

Merged
merged 1 commit into from
Jul 15, 2020

Conversation

vollmerk
Copy link
Contributor

@vollmerk vollmerk commented Jul 8, 2020

Adjust the wording on hosts so that it reflects the value represents the Smart Proxy whose puppet_url will be used, not the FQDN of the Puppet CA or Puppetserver themselves.

Fixes #30351

See - https://community.theforeman.org/t/foreman-provision-template-host-doesnt-have-correct-puppetserver-or-puppetca/19440 for further discussion

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

2 similar comments
@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

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.

Would you mind squashing the commits and following the convention for commit messages?

app/models/concerns/host_common.rb Outdated Show resolved Hide resolved
:label => N_('Puppet Master'),
:description => N_('Use this puppet server as an initial Puppet Server or to execute puppet runs'),
:label => N_('Puppetserver Proxy'),
:description => N_('Use this Smart Proxy to connect to an initial Puppetserver to execute puppet runs'),
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure the initial part is correct. It's also presented in the ENC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the value is specifically used by default during provision - correct? which I'm guessing is where the "initial" originally came from. It doesn't force the use of the defined CA proxy / Puppet Proxy either. hm

"Smart Proxy provided to provisioning templates and ENC as the Puppet Server"

... that sounds awkward... I'll drink some more coffee and keep thinking

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is hard stuff. Perhaps @melcorr has some good thoughts.

IMHO there are 2 things that are relevant. First of all, there's just the intention. Whenever you want to know which server is responsible, this is the authoritative value. Like a CMDB. Other tools and integrations can read this. One of those would be provisioning, but the ENC also sends it so you can correct it. Ansible / REX could also use it to update. The other side is importing classes. That calls the Smart Proxy which in turn calls the Puppetserver API.

Now to summarize that in a short description ...

Copy link
Member

Choose a reason for hiding this comment

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

I might be totally off but it might get you thinking:

Enter the Smart Proxy to use for provisioning templates and as the Puppet Server ENC

or

Enter the Smart Proxy to use as both the default Puppet Server for Puppet runs and the Puppet Server ENC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enter the default Smart Proxy used to access the Puppetserver API

and

Enter the default Smart Proxy used to access the Puppetserver CA API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the latest - let me know if you want me to tweak it anymore - This at least causes me to think/look more and not assume the value I'm picking there is the Server that I want to use, but rather the Smart Proxy... which would when troubleshooting trigger me to look at the Smart Proxy for issues if values aren't correct... So I think this accomplishes the original goal.

:label => N_('Puppet Master'),
:description => N_('Use this puppet server as an initial Puppet Server or to execute puppet runs'),
:label => N_('Puppet Proxy'),
:description => N_('Enter the default Smart Proxy used to access the Puppetserver API'),
Copy link
Member

Choose a reason for hiding this comment

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

To me default implies there's a fallback so I'd leave out default.

But thinking more, how about something like:

Suggested change
:description => N_('Enter the default Smart Proxy used to access the Puppetserver API'),
:description => N_('Use the Puppetserver configured on this Smart Proxy.'),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that better, adjusted

…lue represents the Smart Proxy whose puppet_url will be used
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.

LGTM but since I suggested it I'd like someone else to confirm this makes sense.

@tbrisker
Copy link
Member

ok to test

@ezr-ondrej
Copy link
Member

[test katello]

Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

Thanks @vollmerk ! 👍 sounds like better wording :)

@ezr-ondrej
Copy link
Member

@ekohl
Copy link
Member

ekohl commented Jul 15, 2020

@ezr-ondrej yes, I noticed this too. In theforeman/theforeman.org@cbd3af9 I already updated a few but looks like I wasn't thorough enough.

@ezr-ondrej
Copy link
Member

This can go in regardless, I'll open the PR and let's discuss there :)

@ezr-ondrej ezr-ondrej merged commit 0c9ddcc into theforeman:develop Jul 15, 2020
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.

6 participants