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

Feature: Remove dependency on config files #499

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Feature: Remove dependency on config files #499

wants to merge 2 commits into from

Conversation

sijis
Copy link
Contributor

@sijis sijis commented Jan 8, 2018

This allows a publisher to send a complete configuration via
fedmsg.init().

@sijis
Copy link
Contributor Author

sijis commented Jan 8, 2018

I still need to patch up and create test cases around this change.

I wanted to publish this early enough for feedback.

@codecov
Copy link

codecov bot commented Jan 8, 2018

Codecov Report

Merging #499 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #499   +/-   ##
========================================
  Coverage    60.17%   60.17%           
========================================
  Files           29       29           
  Lines         1750     1750           
  Branches       272      272           
========================================
  Hits          1053     1053           
  Misses         620      620           
  Partials        77       77

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6326189...2db4297. Read the comment docs.

Copy link
Member

@jeremycline jeremycline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on vacation until Wednesday, but at first glance I think this would be better if fedmsg.init switched to the new fedmsg.config.conf dictionary which has sane defaults and validators. You'll likely need to remove those validations of the endpoints setting in fedmsg.config.load_config.

@sijis
Copy link
Contributor Author

sijis commented Jan 8, 2018

I did see the deprecation code in load_config and I wondered why init was still referencing it.

I will convert init to use fedmsg.config.conf.

@sijis sijis mentioned this pull request Jan 8, 2018
@sijis
Copy link
Contributor Author

sijis commented Jan 10, 2018

I'm not sure if this is the correct approach on using FedmsgConfig() in the init() function.

I still have to poke at test cases around this change but it is somewhat related to #500.

if kw:
settings['settings'] = kw

config.load_config(**settings)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than creating a dict with a single key and immediately unpacking it, just do config.load_config(kw).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was waffling back and forth on how to do it. I figured there was a simpler way :). Thanks.

@sijis sijis changed the title [WIP] Feature: Remove dependency on config files Feature: Remove dependency on config files Jan 11, 2018
@jeremycline
Copy link
Member

Could you rebase this onto the latest master? That should take care of the test suite failures.

@sijis
Copy link
Contributor Author

sijis commented Jan 16, 2018

Rebased.

Copy link
Member

@jeremycline jeremycline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this breaks a couple of the tests on Python 2.7 - the validator expects unicode, but it's getting strings.

The fix should be to go to those failing tests which are using invalid config and make sure those strings are prefixed with u. Thanks!

@sijis
Copy link
Contributor Author

sijis commented Jan 16, 2018

I've been poking at this for over an hour and I think the issue is more than just prefixing those strings with u.

There's definitely a discrepancy between fedmsg.config.conf and using the old load_config().

I started poking around and seeing if i could patch https://github.com/fedora-infra/fedmsg/blob/develop/fedmsg/commands/__init__.py#L45, so I could fix the test_logger_basic() test but that's leading me down a rabbit hole.

@jeremycline
Copy link
Member

I poked at this a little bit and ooof, this stuff is messy. The current CLI is pretty awful and should just be tossed out, as should the old config setup... Really the whole fedmsg API.

Honestly, I feel that it's probably not worth trying to get this to work. I don't know what exactly you're using fedmsg for, but if it's not to work with existing Fedora infrastructure I really think you'd be happier using plain ZMQ. It's super simple to use and fedmsg is likely just getting in your way.

That's not to say I won't accept this PR if you can make the current CLI and tests work with the new configuration, but I'm not sure it's the best use of time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants