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/python fedora removal #136

Merged
merged 5 commits into from May 6, 2013

Conversation

Projects
None yet
3 participants
@ralphbean
Contributor

ralphbean commented Apr 30, 2013

The purpose here is to remove the python-fedora dependency of fedmsg by adding two new API functions to fedmsg.meta. Different deployments can implement them differently. For instance, fedora might construct FAS_USERNAME@fedoraproject.org emails from usernames in messages while debian might look them up in a database.

@ralphbean

This comment has been minimized.

Show comment
Hide comment
@ralphbean

ralphbean Apr 30, 2013

Contributor

/cc @laarmen

Contributor

ralphbean commented Apr 30, 2013

/cc @laarmen

@pypingou

This comment has been minimized.

Show comment
Hide comment
@pypingou

pypingou May 4, 2013

Member

Do we need to have a mechanism that checks both places to be backward compatible?

Do we need to have a mechanism that checks both places to be backward compatible?

This comment has been minimized.

Show comment
Hide comment
@ralphbean

ralphbean May 4, 2013

Contributor

Well, since its a user specifiable option, I don't think so. If they have old image files in the gravatar directory they can just specify to use it instead. All that has changed here is the default value.

Contributor

ralphbean replied May 4, 2013

Well, since its a user specifiable option, I don't think so. If they have old image files in the gravatar directory they can just specify to use it instead. All that has changed here is the default value.

Show outdated Hide outdated fedmsg/commands/tail.py
lines = []
for user, obj in itertools.product(users, objs):
_grab_and_cache_avatar(user, cache_directory)
for user, obj in itertools.product(users.keys(), objs):

This comment has been minimized.

@puiterwijk

puiterwijk May 4, 2013

Member

You are no longer assigning users anymore... shouldn't this be avatars.keys()?

@puiterwijk

puiterwijk May 4, 2013

Member

You are no longer assigning users anymore... shouldn't this be avatars.keys()?

This comment has been minimized.

@ralphbean

ralphbean May 4, 2013

Contributor

Yes, you're right!

@ralphbean

ralphbean May 4, 2013

Contributor

Yes, you're right!

def emails(self, msg, **config):
""" Return a dict of emails associated with a message. """
return dict()

This comment has been minimized.

@puiterwijk

puiterwijk May 4, 2013

Member

return - yes
a dict - sure, why not?
of emails - could be...
associated with a message - how??

@puiterwijk

puiterwijk May 4, 2013

Member

return - yes
a dict - sure, why not?
of emails - could be...
associated with a message - how??

This comment has been minimized.

@ralphbean

ralphbean May 4, 2013

Contributor

Yes :)

The method (like the others on BaseProcessor) is supposed to be implemented by child classes in other places. For Fedora Infrastructure messages, we have the fedmsg_meta_fedora_infrastructure module which provides those.

This change here expands that API to allow child classes to return more metadata about messages such as 'what emails are associated with the message' and 'what avatar URLs are associated with this message' in addition to just the 'what usernames are associated with this message' functionality we had before.

The inspiration for this change comes from the Debian community where users are not uniquely identified by something like a FAS username -- they rely much more heavily on the email address of the contributor.

The dict here should be a mapping of usernames (as returned by the usernames method) to email addresses. The avatars method below should be similar: a mapping of usernames (as returned by the usernames method) to avatar urls.

A simple list of emails and avatars might suffice, but the usernames method returns a 'set', not a 'list' (which means that it is unordered). Since we can't rely on the order of the usernames list and the order of the emails list being the same, the dict becomes necessary so users can know whose email is whose.

@ralphbean

ralphbean May 4, 2013

Contributor

Yes :)

The method (like the others on BaseProcessor) is supposed to be implemented by child classes in other places. For Fedora Infrastructure messages, we have the fedmsg_meta_fedora_infrastructure module which provides those.

This change here expands that API to allow child classes to return more metadata about messages such as 'what emails are associated with the message' and 'what avatar URLs are associated with this message' in addition to just the 'what usernames are associated with this message' functionality we had before.

The inspiration for this change comes from the Debian community where users are not uniquely identified by something like a FAS username -- they rely much more heavily on the email address of the contributor.

The dict here should be a mapping of usernames (as returned by the usernames method) to email addresses. The avatars method below should be similar: a mapping of usernames (as returned by the usernames method) to avatar urls.

A simple list of emails and avatars might suffice, but the usernames method returns a 'set', not a 'list' (which means that it is unordered). Since we can't rely on the order of the usernames list and the order of the emails list being the same, the dict becomes necessary so users can know whose email is whose.

import fedmsg.meta
from common import load_config
def skip_on(attributes):

This comment has been minimized.

@puiterwijk

puiterwijk May 4, 2013

Member

So if you specify nothing, you skip? Shouldn't that fail?

@puiterwijk

puiterwijk May 4, 2013

Member

So if you specify nothing, you skip? Shouldn't that fail?

This comment has been minimized.

@ralphbean

ralphbean May 4, 2013

Contributor

Well, I used to just let the test pass with the 'return' statements below.. but that was disingenuous.

There are a set of tests in fedmsg_meta_fedora_infrastructure that test each one of the processors for each of our message types. If I have a test for a bodhi message and declare that: 'the bodhi message processor, given this message, should return 1) this URL 2) this list of packages and 3) this list of usernames' but I don't specify what dict of emails it should return, I don't want that test for emails to fail. The code that test would have tested did not actually fail. It's that the test case was underspecified -- I don't actually want to go through and test that every message processor for every message type returns exactly the same mapping of usernames to fasusername@fedoraproject.org. And so, we skip -- indicating that "this could have been tested, but wasn't".

This gives us a better idea of test coverage at first glance. It doesn't lie and say that tests pass that weren't actually run. But it also makes the test developer's life easier by not requiring him or her to write redundant tests -- they may be omitted.

@ralphbean

ralphbean May 4, 2013

Contributor

Well, I used to just let the test pass with the 'return' statements below.. but that was disingenuous.

There are a set of tests in fedmsg_meta_fedora_infrastructure that test each one of the processors for each of our message types. If I have a test for a bodhi message and declare that: 'the bodhi message processor, given this message, should return 1) this URL 2) this list of packages and 3) this list of usernames' but I don't specify what dict of emails it should return, I don't want that test for emails to fail. The code that test would have tested did not actually fail. It's that the test case was underspecified -- I don't actually want to go through and test that every message processor for every message type returns exactly the same mapping of usernames to fasusername@fedoraproject.org. And so, we skip -- indicating that "this could have been tested, but wasn't".

This gives us a better idea of test coverage at first glance. It doesn't lie and say that tests pass that weren't actually run. But it also makes the test developer's life easier by not requiring him or her to write redundant tests -- they may be omitted.

@puiterwijk

This comment has been minimized.

Show comment
Hide comment
@puiterwijk

puiterwijk May 4, 2013

Member

Okay, those explanations make sense, but you might want to note for some of them things like "# Overloaded" or something (I know there are more of these functions not in the PR, and this suggestion also counts for them), just to document it.

Otheriwse: 👍

Member

puiterwijk commented May 4, 2013

Okay, those explanations make sense, but you might want to note for some of them things like "# Overloaded" or something (I know there are more of these functions not in the PR, and this suggestion also counts for them), just to document it.

Otheriwse: 👍

@ralphbean

This comment has been minimized.

Show comment
Hide comment
@ralphbean

ralphbean May 6, 2013

Contributor

Cool. Just FYI, the class-level docstring notes that the class is meant to be overloaded, and the name is "BaseProcessor" which I think implies that it is meant to be overloaded. I looked and I didn't see a simple obvious way to make than any more explicit, so I won't make any changes.

If and only if you'd like to, feel free to change the language.

Contributor

ralphbean commented May 6, 2013

Cool. Just FYI, the class-level docstring notes that the class is meant to be overloaded, and the name is "BaseProcessor" which I think implies that it is meant to be overloaded. I looked and I didn't see a simple obvious way to make than any more explicit, so I won't make any changes.

If and only if you'd like to, feel free to change the language.

ralphbean added a commit that referenced this pull request May 6, 2013

@ralphbean ralphbean merged commit 6ddce9c into develop May 6, 2013

1 check passed

default The Travis build passed
Details

@ralphbean ralphbean deleted the feature/python-fedora-removal branch Feb 26, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment