-
Notifications
You must be signed in to change notification settings - Fork 20
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
mark *_url settings as required and remove their default values #43
Conversation
89b99ed
to
83e0df0
Compare
These defaults are sort of development values. and I always assumed people would set this explicitly because I don't trust autodetection. |
I think the only way to enforce that is to not have a default value and ensure users think about / set this value. I would tend to prefer no default value other than a default specified in the settings file. |
If there is no sane default, don't have one in the first place. That makes a lot of sense to me. |
I guess the sanity is debatable, yeah. Would y'all prefer if I'd just drop the localhost default values, and mark the settings as required? |
Works for me. Saves doing these lookups regardless of whether a setting is configured (which can fail or slow things down) |
3ae8824
to
d0f3f3b
Compare
8b75895
to
44ad191
Compare
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.
As noted, presence
is redundant. Otherwise 👍
Those URLs are used by Katello and clients to connect to Pulp and `localhost` is almost never the right choice here as it will be either not reachable (for clients) or have the wrong certs (for Katello). Guessing the right URL is hard, so let's not do that and force the user to set it instead.
Why are some errors using "Parameter" and some "Setting"? :D |
Being consistent is hard? |
Those URLs are used by Katello and clients to connect to Pulp and
localhost
is almost never the right choice here as it will be eithernot reachable (for clients) or have the wrong certs (for Katello).
Guessing the right URL is hard, so let's not do that and force the user
to set it instead