-
Notifications
You must be signed in to change notification settings - Fork 93
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
Fix default type check #500
Fix default type check #500
Conversation
fedmsg/config.py
Outdated
@@ -254,8 +254,10 @@ class FedmsgConfig(dict): | |||
'validator': None, | |||
}, | |||
'relay_inbound': { | |||
'default': u'tcp://127.0.0.1:2001', | |||
'validator': _validate_none_or_type(six.text_type), |
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.
It doesn't make much sense to have a relay binding to multiple sockets. Did you run into this by trying to specify a list of values?
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.
I did not run this using multiple entries.
I think the issue is mainly documentation related.
- The existing config show that parameter as a list (https://github.com/fedora-infra/fedmsg/blob/develop/fedmsg.d/relay.py#L36)
- The
fedmsg-config
command also shows this option as a list. - The online docs reference it as a list too (http://fedmsg.readthedocs.io/en/stable/configuration/#relay-inbound)
In my case, I just updated the example config and things worked fine. I never needed to add multiple values to this.
It is very likely I am wrong and reading the wrong documentation parts.
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.
Ah, well the history here is I wrote documentation. There wasn't any documentation before and I took wild guesses. Recently I tried to add defaults to every setting and picked what I thought were reasonable defaults.
Although it looks like lists and strings are both handled correctly, it's not a setting that makes a bunch of sense as a list so I'm in favor of changing the documentation and config in fedmsg.d
(really, that directory should go away, but I have no doubt that'd break something weird relying on it 😢 )
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.
I will definitely change/revert that list change in this PR.
I could also include documentation changes so that it references it as a string, not list. This would keep everything consistent, at least.
At quick check, tests fail removing the fedmsg.d
directory. I bet packaging would have issues too. Although, both of those could be solved.
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.
I've removed this commit completely.
Updated references of |
Codecov Report
@@ Coverage Diff @@
## develop #500 +/- ##
===========================================
- Coverage 60.28% 60.17% -0.12%
===========================================
Files 29 29
Lines 1750 1750
Branches 272 272
===========================================
- Hits 1055 1053 -2
- Misses 618 620 +2
Partials 77 77
Continue to review full report at Codecov.
|
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.
Sorry, I should have made an note on the other changes on the first review. One small thing, but other than that it looks really good, thanks!
fedmsg/config.py
Outdated
@@ -294,8 +294,8 @@ class FedmsgConfig(dict): | |||
'validator': _validate_none_or_type(six.text_type), | |||
}, | |||
'crl_cache_expiry': { | |||
'default': None, | |||
'validator': _validate_none_or_type(six.text_type), | |||
'default': 10, |
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.
The docs say this is in seconds (although the docs are a little unreliable) so 10 is a really small default for a CRL. Maybe an hour is more reasonable?
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.
Good point. Updating...
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.
I think you missed updating it in a few places? Additionally, an hour would be 60*60 (3600) since this is seconds, not minutes.
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.
You don't want to to expire in 1 minute? 😂
Let me try this again.
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.
Totally missed several spots.
Alright I think it finally got this.
Also, sorry for the constant rebasing. Just trying to keep clean commits.
Great, thanks for this! |
While working on #499, I noticed some discrepancies in the validation of the configs in
FedmsgConfig
.This attempts to address those discrepancies.
I used the generated output from
fedmsg-config
as the source of truth on what these values should be.