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 a severity to messages #48

Merged
merged 1 commit into from
Aug 28, 2018
Merged

Conversation

jeremycline
Copy link
Member

Each message can now have a severity associated with it, both at a class
level and on a message-by-message basis.

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

@jeremycline jeremycline requested a review from a team as a code owner August 27, 2018 18:38
@codecov-io
Copy link

codecov-io commented Aug 27, 2018

Codecov Report

Merging #48 into master will decrease coverage by 2.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
- Coverage   91.85%   89.72%   -2.14%     
==========================================
  Files          11       11              
  Lines         786      798      +12     
  Branches      107      109       +2     
==========================================
- Hits          722      716       -6     
- Misses         42       56      +14     
- Partials       22       26       +4
Impacted Files Coverage Δ
fedora_messaging/message.py 100% <100%> (+2.75%) ⬆️
fedora_messaging/cli.py 66.17% <0%> (-30.89%) ⬇️
fedora_messaging/_session.py 81.08% <0%> (-0.26%) ⬇️
fedora_messaging/api.py 78.26% <0%> (ø) ⬆️

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 54d704e...2aa0a98. Read the comment docs.

Each message can now have a severity associated with it, both at a class
level and on a message-by-message basis.

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

@cverna cverna left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link

@puiterwijk puiterwijk left a comment

Choose a reason for hiding this comment

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

While I believe that the specific values of severities might be hard to determine, this code looks reasonable to me.

@jeremycline
Copy link
Member Author

While I believe that the specific values of severities might be hard to determine, this code looks reasonable to me.

Do you mean for a particular message it's difficult to say what the severity is? I agree and I expect we'll have plenty of spirited discussions stemming from someone getting notifications they're not interested it. It's not perfect, but hopefully that will be a guiding force for the "right" severity level.

@abompard, I think I'm going to go ahead and merge this for a beta release (which includes all the fixed-up auth stuff), but when you get back from PTO, please review this and we can adjust it before a stable release is tagged.

@jeremycline jeremycline merged commit 2aa0a98 into fedora-infra:master Aug 28, 2018
@jeremycline jeremycline deleted the severity branch August 28, 2018 19:05
Copy link
Member

@abompard abompard left a comment

Choose a reason for hiding this comment

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

This looks good! Should the tutorial be updated to ask people to set the default severity?

_log.debug(
'Validating message body "%r" with schema "%r"', self._body, schema
)
jsonschema.validate(self._body, schema)
Copy link
Member

Choose a reason for hiding this comment

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

You're doing this so people don't override the base schemas too much? Are you thinking of a specific use case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that for the headers, it'd be good to validate we added the expected schema, timestamp, etc. keys and the easiest way would be to add it to the schema, but then I wanted people to have the ability to expand upon it. With the body I think it's reasonable to demand all messages be an object, so this makes it enforced.

I can't really think of a use-case for users to actually add headers at the moment so maybe that should all be an internal thing, though.

msg = message.Message()
msg._headers = None
expected_message = message.Message()
expected_message.id = msg.id
Copy link
Member

Choose a reason for hiding this comment

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

You don't seem to be using this variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good eye

@jeremycline
Copy link
Member Author

This looks good! Should the tutorial be updated to ask people to set the default severity?

I'll do that

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.

5 participants