-
Notifications
You must be signed in to change notification settings - Fork 96
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
remove superfluous --certs-update-server
option
#3136
Conversation
This option is both unnecessary and causes the command to fail
The PR preview for d13b5a9 is available at theforeman-foreman-documentation-preview-pr-3136.surge.sh The following output files are affected by this PR: |
@dschlenk Do you have the error handy? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a specific error message when you see it fail?
Looking at the code (https://github.com/theforeman/foreman-installer/blob/e4fc6826b87177e7629258ea8b0499ebc0b559ea/hooks/pre/20-certs_update.rb#L7-L26) I can imagine it fails if the build directory doesn't exist yet.
Having said that, I've been wanting to get rid of the whole parameter anyway. IMHO our code should detect if it needs an update and behave accordingly. It's always been a hack, but I don't know if our code is now robust enough to not need it anymore.
The way it works is that we force a generation if the update file exists and afterwards remove it:
https://github.com/theforeman/puppet-certs/blob/94b2b3ecf3a2365f8db2ae965c84b3d2462359bc/lib/puppet/provider/katello_ssl_tool.rb#L43
https://github.com/theforeman/puppet-certs/blob/94b2b3ecf3a2365f8db2ae965c84b3d2462359bc/lib/puppet/provider/katello_ssl_tool.rb#L37
It looks like we simply look for the existence of the files, not the actual content.
So I'd say it's safe to drop in this procedure.
And we can make the code more robust: theforeman/foreman-installer#952
@ehelms yup:
FWIW the instructions for a default TLS certificate do not have this option in the corresponding command. The help text for the option is |
If we drop this option, and the certs do change, I'm not convinced the new certificates will get deployed into the bundle. I think that is why we always had that option included, to make sure it always ensures the latest version of the custom SSL certificates gets laid down and thus in the bundle. |
@ehelms the command in the certificate renewal section retains the option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution; diff LGTM.
This option is both unnecessary and causes the command to fail. (cherry picked from commit bc36214)
This option is both unnecessary and causes the command to fail. (cherry picked from commit bc36214)
This option is both unnecessary and causes the command to fail. (cherry picked from commit bc36214)
This option is both unnecessary and causes the command to fail
Please cherry-pick my commits into: