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

WIP: Initial message schema for Anitya #570

Merged
merged 3 commits into from Jan 30, 2019

Conversation

Projects
None yet
3 participants
@jeremycline
Copy link
Member

jeremycline commented Aug 1, 2018

This defines message schema using json-schema for a subset of messages
sent by Anitya.

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

@jeremycline jeremycline force-pushed the jeremycline:message-schema branch from df9292c to 832e387 Aug 1, 2018

@jeremycline jeremycline changed the title Initial message schema for Anitya WIP: Initial message schema for Anitya Aug 2, 2018

@Zlopez

This comment has been minimized.

Copy link
Member

Zlopez commented Oct 11, 2018

As Fedora-messaging is released with version 1.0.0, this could be a good addition to the project.

@jeremycline
Do you need any help with this PR?

@jeremycline

This comment has been minimized.

Copy link
Member Author

jeremycline commented Oct 11, 2018

I'm going to take a bit of time today or tomorrow to clean it up, it'll be a good first app to move over

@jeremycline jeremycline force-pushed the jeremycline:message-schema branch 2 times, most recently from b0f4677 to d913085 Oct 15, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 15, 2018

Codecov Report

Merging #570 into master will decrease coverage by 6.34%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #570      +/-   ##
==========================================
- Coverage   96.49%   90.14%   -6.35%     
==========================================
  Files          56       56              
  Lines        2909     2690     -219     
  Branches      392      352      -40     
==========================================
- Hits         2807     2425     -382     
- Misses         64      201     +137     
- Partials       38       64      +26
Impacted Files Coverage Δ
anitya/ui.py 77.62% <0%> (-20.86%) ⬇️
anitya/lib/backends/drupal6.py 75% <0%> (-18.55%) ⬇️
anitya/lib/backends/drupal7.py 75% <0%> (-18.55%) ⬇️
anitya/lib/utilities.py 80.1% <0%> (-14.99%) ⬇️
anitya/lib/backends/pecl.py 87.23% <0%> (-12.77%) ⬇️
anitya/db/models.py 87.8% <0%> (-12.2%) ⬇️
anitya/lib/backends/bitbucket.py 80.95% <0%> (-11.36%) ⬇️
anitya/admin.py 84.25% <0%> (-7.44%) ⬇️
anitya/lib/backends/pear.py 95.74% <0%> (-4.26%) ⬇️
anitya/lib/backends/packagist.py 90% <0%> (-2.6%) ⬇️
... and 28 more

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 fcbc0cc...02c7208. Read the comment docs.

@jeremycline jeremycline force-pushed the jeremycline:message-schema branch from d913085 to 9722666 Oct 15, 2018

@Zlopez

This comment has been minimized.

Copy link
Member

Zlopez commented Oct 16, 2018

I looked at the failing Travis job and it failed, because it took to long. The last test suite running was test_project.py. There must be something that caused infinite loop or something similar.

The docs test is failing because it doesn't know fedora-messaging.

ModuleNotFoundError: No module named 'fedora_messaging'
@jeremycline

This comment has been minimized.

Copy link
Member Author

jeremycline commented Oct 16, 2018

Yup, I'm not done with this yet, sorry for the noise

@jeremycline jeremycline force-pushed the jeremycline:message-schema branch from 9722666 to 147f9d7 Oct 16, 2018

jeremycline added some commits Aug 1, 2018

Initial message schema for Anitya
This defines message schema using json-schema for the messages Anitya
sends. It does not, however, implement any Python API work with these
messages.

Signed-off-by: Jeremy Cline <jcline@redhat.com>
Publish messages via fedora_messaging
This is the bare minimum to publish messages using fedora_messaging.
It includes support to use the fedmsg publisher as a config option in
case things go wrong during deployment. This can be removed later.

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

@jeremycline jeremycline force-pushed the jeremycline:message-schema branch from 147f9d7 to a893d6e Dec 12, 2018

@jeremycline

This comment has been minimized.

Copy link
Member Author

jeremycline commented Dec 12, 2018

This depends on #635 which I think depended on #633. Tests are failing, but a lot of them have to do with the logging code that's about to be removed, so I don't want to spend time fixing them just to remove them.

@Zlopez

This comment has been minimized.

Copy link
Member

Zlopez commented Dec 13, 2018

Ok, I will than fix #635 first than.

@Zlopez
Copy link
Member

Zlopez left a comment

It looks good I just have a few comments

@@ -38,19 +40,29 @@

def fedmsg_publish(*args, **kwargs): # pragma: no cover

This comment has been minimized.

@Zlopez

Zlopez Dec 14, 2018

Member

I think it will be good to rename this function to publish

This comment has been minimized.

@jeremycline

jeremycline Dec 14, 2018

Author Member

This is a interim solution. The most reasonable refactor is likely to get rid of this function entirely. The reason is that different messages almost certainly have different failure scenarios and once you get rid of the fedmsg portion of this function, all it's doing is squashing exceptions.

@@ -177,68 +180,66 @@ def create_distro(session):

def create_project(session):
""" Create some basic projects to work with. """
utilities.create_project(

This comment has been minimized.

@Zlopez

Zlopez Dec 14, 2018

Member

I'm glad to see you are getting rid of the utilities functions. I want to remove most of them in the future.

"type": "object",
"required": ["project", "message", "distro"],
"properties": {
"distro": {"type": "null"},

This comment has been minimized.

@Zlopez

Zlopez Dec 14, 2018

Member

Why are we sending this in project related messages?

This comment has been minimized.

@jeremycline

jeremycline Dec 14, 2018

Author Member

So these schema aren't what we should be sending, it's just what the current messages look like. Most of the message schema are crazy and make no sense. This is more about documenting the madness so we can fix it than it is about laying out a reasonable set of messages. Once we've done that you can come up with reasonable messages and a migration plan for consumers.

"type": "object",
"properties": {
"agent": {"type": "string"},
"odd_change": {"type": "boolean"},

This comment has been minimized.

@Zlopez

Zlopez Dec 14, 2018

Member

This is no longer happening. I removed this error when I introduced check for multiple versions.

This comment has been minimized.

@jeremycline

jeremycline Dec 14, 2018

Author Member

That's not good :(

The message schema changed and it's possible consumers were relying on that field. If, for example, the-new-hotness expects this key to be present it will crash. The best path forward is likely to make sure that key remains the message getting sent out and is always false or something.

This comment has been minimized.

@Zlopez

Zlopez Dec 14, 2018

Member

Oh, I didn't knew this could have impact like this. The change is only on master right now. It wasn't released yet, so I can add the field back.
It's hard to know what is required.

This comment has been minimized.

@Zlopez

Zlopez Jan 23, 2019

Member

The odd_change is now always false.

@@ -0,0 +1,20 @@
# Copyright (C) 2018 Red Hat, Inc.

This comment has been minimized.

@Zlopez

Zlopez Dec 14, 2018

Member

Fixtures are really nice solution for testing.

The fixtures are generated by downloading messages from datagrepper.
"""

def test_valid(self):

This comment has been minimized.

@Zlopez

Zlopez Dec 14, 2018

Member

I don't see any assert in this method.

This comment has been minimized.

@jeremycline

jeremycline Dec 14, 2018

Author Member

Yep, that's expected, although I should probably add a comment to that effect. validate() raises an exception if the message is invalid which would cause the test to fail. The assertion is a basically there there isn't an exception.



setup(
name="anitya_schema",

This comment has been minimized.

@Zlopez

Zlopez Dec 14, 2018

Member

I expect that this should be the package I have to import in the-new-hotness.

This comment has been minimized.

@jeremycline

jeremycline Dec 14, 2018

Author Member

Correct, it'll be published to PyPI/packaged separately from anitya so consumers can just pull in the schema

@Zlopez Zlopez referenced this pull request Dec 14, 2018

Merged

Return odd_change parameter #682

@Zlopez

This comment has been minimized.

Copy link
Member

Zlopez commented Jan 8, 2019

@jeremycline All the blocking PRs should be merged now, so you can continue on this PR when you have time.

@Zlopez

This comment has been minimized.

Copy link
Member

Zlopez commented Jan 30, 2019

I'm merging this and continue working where @jeremycline ended.

@Zlopez Zlopez merged commit b6c1568 into release-monitoring:master Jan 30, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
LGTM analysis: JavaScript No alert changes
Details
LGTM analysis: Python No alert changes
Details
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.