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

Drop parent_fqdn in favor of deriving from foreman_url #309

Merged
merged 1 commit into from
Dec 21, 2020

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Dec 16, 2020

I wanted to continue working toward the number of things a user has to specify to get things working and in this case the number of things a user has to specify multiple times, via different parameters. If we take https://github.com/theforeman/forklift/blob/master/pipelines/install/04-install_proxy.yaml#L11 as an example, the user effectively specifies the Foreman server hostname 4 times in the most basic of commands to install a content proxy.

manifests/init.pp Outdated Show resolved Hide resolved
@ehelms
Copy link
Member Author

ehelms commented Dec 17, 2020

Now with a function for deriving the hostname by parsing the URL. Admittedly, I couldn't come up with a better name than hostname for the function. Open to suggestions if others don't like it.

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.

url2hostname perhaps?

lib/puppet/functions/foreman_proxy_content/hostname.rb Outdated Show resolved Hide resolved
spec/functions/foreman_proxy_content/hostname_spec.rb Outdated Show resolved Hide resolved
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.

Looking at the URI docs and RFC I think we should use host. hostname implies it's a name, but technically an IP is also valid. Yes, I know that ip: is rare in subjectAltName, but let's follow the standard. We can also handle that fine in the other code.

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.

2 small nits, otherwise 👍

manifests/init.pp Outdated Show resolved Hide resolved
@ekohl
Copy link
Member

ekohl commented Dec 17, 2020

This is backwards incompatible and we will need to handle this in forklift where we pass it in our CI. Do you want to wait for that PR to be ready before we merge it?

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