Fix default type check#500
Fix default type check#500jeremycline merged 5 commits intofedora-infra:developfrom sijis:fix/default_type_checks
Conversation
| }, | ||
| '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.
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.
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-configcommand 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.
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.
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.
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.
|
jeremycline
left a comment
jeremycline
left a comment
There was a problem hiding this comment.
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
| 'crl_cache_expiry': { | ||
| 'default': None, | ||
| 'validator': _validate_none_or_type(six.text_type), | ||
| 'default': 10, |
There was a problem hiding this comment.
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.
Good point. Updating...
There was a problem hiding this comment.
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.
You don't want to to expire in 1 minute? 😂
Let me try this again.
There was a problem hiding this comment.
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! |
2 participants
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-configas the source of truth on what these values should be.