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

Add the bodhi_messages Python package #2842

Merged
merged 1 commit into from Mar 22, 2019

Conversation

@jeremycline
Copy link
Member

commented Dec 12, 2018

fedora-messages uses Python classes to allow authors to define JSON
schema and a Python API to work with their messages. This adds the
basics of a Python package to distribute these schema.

This is similar to fedora_infrastructure_fedmsg_meta, except each
application manages its own schema definitions and how the message is
represented to the user.

Signed-off-by: Jeremy Cline jcline@redhat.com


I had this sitting in a branch and meant to write schema for Bodhi, but I've not found time yet. I thought it best to push what I have (which is just a skeleton package) and have us all chip away and defining schema and changing the code that publishes messages to use them.

I pulled the list of messages from https://fedora-fedmsg.readthedocs.io/en/latest/, but some may not still be sent. Also that list might not actually be complete. I expect as we audit the code we'll alter the list of classes here.

@jeremycline jeremycline requested a review from fedora-infra/bodhi as a code owner Dec 12, 2018

@abompard abompard force-pushed the jeremycline:fml-schema branch from 01fca03 to 31b3b78 Dec 15, 2018

@abompard

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

Hey guys, what do you think of this now?

@abompard abompard force-pushed the jeremycline:fml-schema branch 2 times, most recently from 9392ab7 to 0b1251b Dec 17, 2018

@bowlofeggs
Copy link
Member

left a comment

I like what I see here. Can we squash some of the commits to be more atomic? The flake8 commit is the main one that stands out as being sensible to squash.

Also I think we should rebase, because the first commit seems to be merged into develop.

@jeremycline are you satisfied with all the other changes here?

"""Sent when a stack is deleted."""

topic = "bodhi.stack.delete"
_summary_tmpl = "{agent} deleted the \"{name}\" stack"

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Jan 3, 2019

Member

I'm sad to say that I'm actually planning on dropping stacks as part of bodhi 4.0.0. That said, no harm in accepting this change, it's just sad because I'll be removing it really soon.

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Feb 4, 2019

Member

Stacks is actually dropped now, so I will drop this code before merging.

"License :: OSI Approved :: GNU General Public License v2 or later (GPLv2+)",
"Operating System :: POSIX :: Linux",
"Programming Language :: Python :: 2",
"Programming Language :: Python :: 2.7",

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Jan 3, 2019

Member

Is there a universe where I can avoid adding a new Python 2 package you think? Do you think it would be fair for me to say that any packages that want to consume Bodhi's cool new messages need to also use Python 3? @hroncok might be happiest if we can be pure Python 3 here.

I've got an open pull request that drops Python 2 support from the server, FWIW: #2759.

This comment has been minimized.

Copy link
@hroncok

hroncok Jan 3, 2019

Contributor

I mean, do whatever you want upstream, just don't put the Python 2 part to Fedora ever :)

That said, Python 3 only upstream works best, because nobody is tempted to package it into Fedora as a python2 package :D

This comment has been minimized.

Copy link
@jeremycline

jeremycline Jan 8, 2019

Author Member

You'll only need Python 2 one or more Bodhi message consumers require it. I'm not sure what all consumes Bodhi messages, but it probably wouldn't hurt to start with Python 3 only.

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Jan 15, 2019

Member

I say let's just force Python 3 for people who want to use this package. That way I don't have to do a big deprecation/removal in the future. People who aren't ready to switch can just keep using the fedmsg relay.

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Feb 4, 2019

Member

I'm going to edit this commit to make it be Python 3 only.

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Feb 4, 2019

Member

I'm adding this patch on top of the first commit:

diff --git a/bodhi_messages/setup.py b/bodhi_messages/setup.py
index 4dcbc521..b5a55bb5 100644
--- a/bodhi_messages/setup.py
+++ b/bodhi_messages/setup.py
@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/python3
 #
 # Copyright (C) 2018  Red Hat, Inc.
 #
@@ -35,11 +35,7 @@ setup(
     classifiers=[
         "License :: OSI Approved :: GNU General Public License v2 or later (GPLv2+)",
         "Operating System :: POSIX :: Linux",
-        "Programming Language :: Python :: 2",
-        "Programming Language :: Python :: 2.7",
         "Programming Language :: Python :: 3",
-        "Programming Language :: Python :: 3.4",
-        "Programming Language :: Python :: 3.5",
         "Programming Language :: Python :: 3.6",
         "Programming Language :: Python :: 3.7",
     ],

I made the shebang opinionated about Python 3 and I also opted to only support the versions of Python 3 in current supported Fedora releases, since I currently only "support" Fedora as an installation target.

@jeremycline

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

@jeremycline are you satisfied with all the other changes here?

It looks like a solid start to me. Once this is merged the work remaining would be:

  • Add JSON schema to each class to validate messages

  • Go through the Bodhi code and create the various message classes rather than publishing via bodhi.notifications using a generic message object.

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

The f29-pydocstyle test actually passed, it seems that it was just the GitHub notifier itself that failed (weird).

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

I'm going to rebase this and force push.

@bowlofeggs bowlofeggs force-pushed the jeremycline:fml-schema branch 2 times, most recently from 8025db9 to 6bae7eb Feb 4, 2019


setup(
name="bodhi_messages",
version="1.0.0",

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Feb 4, 2019

Member

This opens the door to me needing to solve a problem I currently have. Until now, the Bodhi server and client packages are versioned together in lock-step. This has advantages and disadvantages. The main advantage is that it's easy and pretty clean. The main disadvantage is that CLI users often have to wait for a new Fedora release just to get new features when I make X releases for the server, even though the CLI itself rarely has backwards incompatible changes.

There are two alternatives I've been considering. One is what you have done here - give each package its own setup.py and version. The advantage is that it allows me to version each component separately, while also allowing me to make single commits that adjust each of them. The key to the first advantage is that it allows me to be more granular with my semantic versioning. Sometimes I make an X release that doesn't really affect certain classes of users (like CLI or soon to be fedora messaging users). The downside is that I'll have to start tagging them and branching them separately, which might be very strange (especially the branching). (The setup.py I've got now is suuuuuper incorrect anyway, and just abuses how setuptools works and uses it in an undocumented manner.)

Another option I've considered is giving each component its own git repo. This way the branching model and tags can remain sensible. The downside is that changes that affect both now have to coordinate across repos (and so of course can't be one commit), and it's more complex to manage three repositories instead of one.

The branching problem does strike me as a tricky one. I currently make X.Y branch names when I want to release, and then cherry pick patches into it as needed. If we have three components with three X.Y's, it's not clear how that would be sensible anymore (unless I just say that the X.Y is for the server version and ignore the weirdness of the other two components. Maybe the weirdness isn't so bad). I don't know how much I love the extra complication of having three repos.

I'm a little inclined to say that all of these concerns should be separate of this pull request since this pull request is about adding schemas to the messages. Would it be OK for now if we just made this do what the CLI does and do even more weirdness in the setup.py for the sake of keeping this pull request more atomic? If we want to break these components out (which I'm not opposed to), it might be nice to do in a separate PR, especially since I'd like to do it in a consistent fashion for the CLI and base package. An alternative is to make it another commit on this PR if it's important to you to start off with version 1.0.0.

What do you guys think?

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Feb 4, 2019

Member

See also: #2401

"License :: OSI Approved :: GNU General Public License v2 or later (GPLv2+)",
"Operating System :: POSIX :: Linux",
"Programming Language :: Python :: 2",
"Programming Language :: Python :: 2.7",

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Feb 4, 2019

Member

I'm adding this patch on top of the first commit:

diff --git a/bodhi_messages/setup.py b/bodhi_messages/setup.py
index 4dcbc521..b5a55bb5 100644
--- a/bodhi_messages/setup.py
+++ b/bodhi_messages/setup.py
@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/python3
 #
 # Copyright (C) 2018  Red Hat, Inc.
 #
@@ -35,11 +35,7 @@ setup(
     classifiers=[
         "License :: OSI Approved :: GNU General Public License v2 or later (GPLv2+)",
         "Operating System :: POSIX :: Linux",
-        "Programming Language :: Python :: 2",
-        "Programming Language :: Python :: 2.7",
         "Programming Language :: Python :: 3",
-        "Programming Language :: Python :: 3.4",
-        "Programming Language :: Python :: 3.5",
         "Programming Language :: Python :: 3.6",
         "Programming Language :: Python :: 3.7",
     ],

I made the shebang opinionated about Python 3 and I also opted to only support the versions of Python 3 in current supported Fedora releases, since I currently only "support" Fedora as an installation target.

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

I talked with @jeremycline about this pull request and he seemed OK with me changing it to use the same setup.py we use for the other subpackages. What do you think, @abompard?

@abompard

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

I don't really have an opinion on that, as long as it produces a separate installable python package, I'm fine with it.

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

OK. I'm going to move bodhi_messages into bodhi/messages and merge its setup.py with the main one for now, contributing further to the horror that lies within that file. I am interested in finding a better way in the future, but I'm not sure what the best way is at this time.

@bowlofeggs bowlofeggs force-pushed the jeremycline:fml-schema branch from 6bae7eb to ded75f1 Mar 11, 2019

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Alright, I've pushed up a commit that moves things around in the way I proposed earlier. It will not pass CI until #2987 is merged, however.

@abompard @jeremycline - if I could get a +1 from one of you, I'll take it from here (I'm going to squash all the commits into 1 if that's OK with you) and I'll merge this after I can rebase it upon #2987.

@bowlofeggs bowlofeggs force-pushed the jeremycline:fml-schema branch from ded75f1 to 6e28244 Mar 11, 2019

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

I just pushed up another change to my commit - I removed the stacks messages because stacks got dropped a while ago. I renamed mash to compose since that change also happened recently. Lastly, I moved the tests into a schema/ folder since the schemas they are testing are in a folder by that name.

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

I added one more commit to make pydocstyle happy.

@abompard

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Yep, +1 from me.

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Turns out there are still issues with coverage on this pull request, so I'll add some more commits to bump up the coverage.

"""
username = self._body.get('agent', None)
if isinstance(username, dict):
username = username['name']

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Mar 12, 2019

Member

@abompard this pattern above is found in several of the message classes and is causing coverage issues because the test messages don't generally have users expressed this way. This code was copied from the old fedmsg code, right? Is it likely that the messages were once this way, and so this code was there for a transition long ago, and Bodhi doesn't encode usernames this way anymore? How complete are the set of messages in the tests? Do they include one of every type of message that Bodhi sends today?

This comment has been minimized.

Copy link
@abompard

abompard Mar 12, 2019

Member

Yeah it was copied from old fedmsg code, you can very probably drop it, those messages may not be going around on the bus anymore.

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Mar 15, 2019

Member

OK - I'm currently going through the tests and making them be consistent with the data structures I see in datagrepper. I am dropping the tests that check "legacy" data.

@bowlofeggs bowlofeggs force-pushed the jeremycline:fml-schema branch 2 times, most recently from c2d7e51 to 111c089 Mar 14, 2019

@bowlofeggs bowlofeggs self-assigned this Mar 15, 2019

@bowlofeggs bowlofeggs added this to To do in CI Gating via automation Mar 15, 2019

@bowlofeggs bowlofeggs added this to In progress in Release Bodhi 4.0.0 via automation Mar 15, 2019

@bowlofeggs bowlofeggs moved this from To do to In progress in CI Gating Mar 15, 2019

@bowlofeggs bowlofeggs moved this from In progress to Needs review in Release Bodhi 4.0.0 Mar 15, 2019

@bowlofeggs bowlofeggs force-pushed the jeremycline:fml-schema branch from 96d08d9 to 1aa482c Mar 20, 2019

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Alright, one more day that I didn't get quite done, but there are only either 0 or 2 schemas left to write, depending on what we think about my question on the sync script messages.

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

After sleeping on it, I'm going to drop these schemas. Bodhi doesn't send them, so Bodhi can't know when/if they change. It's not sensible for Bodhi to own them.

@bowlofeggs bowlofeggs force-pushed the jeremycline:fml-schema branch 5 times, most recently from f38c54c to d602b90 Mar 20, 2019

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Both of the CI failures are due to hitting postgres startup timeouts. I filed #3074 about that problem. The tests all pass locally for me.

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

This is now ready for review. Could one of @abompard or @jeremycline confirm that this is sane?

I'd prefer to squash this whole thing down to one commit, if that's OK with both of you. When doing so, I will indicate that all three of us authored it. I haven't squashed it yet so it'll be easier for you to see which changes I made.

@bowlofeggs bowlofeggs moved this from In progress to Need Review in CI Gating Mar 20, 2019

@abompard
Copy link
Member

left a comment

Looks very nice! Just a few questions.


if 'comment' in self.body:
text = self.body['comment']['text']
mentions = re.findall(r'@\w+', text)

This comment has been minimized.

Copy link
@abompard

abompard Mar 20, 2019

Member

I suppose you'll get domain names if somebody adds an email address to comments, or an URI or something like it. I think there's a way to say that the mention must start with or at a word boundary, but otherwise you could put something like ^|\s+ before the @ maybe? I'm not sure.

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Mar 21, 2019

Member

So Bodhi actually already has an RE for this that seems to work, but it's part of the server code:

https://github.com/fedora-infra/bodhi/blob/3.13.3/bodhi/server/ffmarkdown.py#L33

However, the server and this new message package both use the bodhi base package, which to date has no code. I think I'll just move that RE to live in bodhi/init.py and have the other two use it from there.

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Mar 21, 2019

Member

Also, I wrote a test that uses an e-mail address, and you are right - it does match that. Using the other RE seems to work as expected.

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Mar 21, 2019

Member

Also, FTR, I didn't write this code - I think it came from wherever this message code lived before this ☺

This comment has been minimized.

Copy link
@abompard

abompard Mar 21, 2019

Member

As they say : "... and now you have two problems" ;-)

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Mar 21, 2019

Member

@abompard I pushed up a new commit to fix this, and it includes a test to ensure it stays fixed.

bodhi/messages/schemas/base.py Show resolved Hide resolved
@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

This is ready for re-re-re-re-re-review.

@abompard
Copy link
Member

left a comment

Yep, looks good! :-)

@@ -17,3 +17,6 @@
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
"""Fedora's update manager."""

# This is a regular expression used to match username mentions in comments.
MENTION_RE = r'(?<!\S)(@\w+)'

This comment has been minimized.

Copy link
@abompard

abompard Mar 21, 2019

Member

Well you could have re.compile'd the regexp right here and gained execution speed but that's really a detail ;-)

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Mar 21, 2019

Member

Good idea, I'll make that so.

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Mar 21, 2019

Member

I'm applying this patch before merging:

diff --git a/bodhi/__init__.py b/bodhi/__init__.py
index 9d79ab1d..d08279bb 100644
--- a/bodhi/__init__.py
+++ b/bodhi/__init__.py
@@ -18,5 +18,8 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 """Fedora's update manager."""
 
+import re
+
+
 # This is a regular expression used to match username mentions in comments.
-MENTION_RE = r'(?<!\S)(@\w+)'
+MENTION_RE = re.compile(r'(?<!\S)(@\w+)')

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Mar 21, 2019

Member

Actually I'm going to unapply that patch - ffmarkdown seems to really want text. Also, Python does cache recently used regular expressions, so I think we effectively get the same performance here anyway since we don't use any other regular expressions in the package.

@bowlofeggs bowlofeggs force-pushed the jeremycline:fml-schema branch 2 times, most recently from ade4471 to 017285f Mar 21, 2019

@bowlofeggs bowlofeggs removed the no-mergify label Mar 21, 2019

Release Bodhi 4.0.0 automation moved this from Needs review to Reviewer approved Mar 21, 2019

@bowlofeggs bowlofeggs force-pushed the jeremycline:fml-schema branch from 017285f to dde4781 Mar 21, 2019

Add the bodhi.messages Python package
fedora-messages uses Python classes to allow authors to define JSON
schema and a Python API to work with their messages. This adds the
basics of a Python package to distribute these schema.

This is similar to fedora_infrastructure_fedmsg_meta, except each
application manages its own schema definitions and how the message is
represented to the user.

Signed-off-by: Jeremy Cline <jcline@redhat.com>
Signed-off-by: Aurélien Bompard <aurelien@bompard.org>
Signed-off-by: Randy Barlow <randy@electronsweatshop.com>

@bowlofeggs bowlofeggs force-pushed the jeremycline:fml-schema branch from 1dc126c to a802453 Mar 21, 2019

@mergify mergify bot merged commit 82c8926 into fedora-infra:develop Mar 22, 2019

44 checks passed

DCO DCO
Details
Mergify — Summary 1 rule matches and 2 potential rules
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
f28-build
Details
f28-diff-cover
Details
f28-docs
Details
f28-flake8
Details
f28-integration
Details
f28-integration-build
Details
f28-pydocstyle
Details
f28-unit
Details
f29-build
Details
f29-diff-cover
Details
f29-docs
Details
f29-flake8
Details
f29-integration
Details
f29-integration-build
Details
f29-pydocstyle
Details
f29-unit
Details
f30-build
Details
f30-diff-cover
Details
f30-docs
Details
f30-flake8
Details
f30-integration
Details
f30-integration-build
Details
f30-pydocstyle
Details
f30-unit
Details
pip-build
Details
pip-diff-cover
Details
pip-docs
Details
pip-flake8
Details
pip-integration
Details
pip-integration-build
Details
pip-mypy
Details
pip-pydocstyle
Details
pip-unit
Details
rawhide-build
Details
rawhide-diff-cover
Details
rawhide-docs
Details
rawhide-flake8
Details
rawhide-integration
Details
rawhide-integration-build
Details
rawhide-pydocstyle
Details
rawhide-unit
Details

CI Gating automation moved this from Need Review to Merged to develop Mar 22, 2019

Release Bodhi 4.0.0 automation moved this from Reviewer approved to Done Mar 22, 2019

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented May 8, 2019

This patch is planned for inclusion in the upcoming 4.0.0 release: #3221

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented May 10, 2019

@cverna cverna moved this from Merged to develop to Deployed to production in CI Gating May 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.