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

ZMQMessage isn't iterable, use __dict__ #462

Merged
merged 2 commits into from Aug 16, 2017

Conversation

Projects
None yet
4 participants
@relrod
Member

relrod commented Aug 9, 2017

This was flooding logs with traces after we did updates on 2017-08-09. This is the hotfix I came up with, but if it's fixed another way that's fine with me too. Just wanted to get this out there.

@relrod relrod requested a review from ralphbean Aug 9, 2017

@relrod

This comment has been minimized.

Member

relrod commented Aug 9, 2017

It looks like sometimes this method gets called with a dict and other times with a ZMQMessage? Or the tests are wrong. It was definitely a ZMQMessage in the traces we were seeing, but the tests seem to pass it a dict (which is why they failed).

@jeremycline

This comment has been minimized.

Member

jeremycline commented Aug 9, 2017

Oof. I think the tests are wrong, but because the API isn't defined at all I have no idea. I'll do some research on this.

@jeremycline

This comment has been minimized.

Member

jeremycline commented Aug 9, 2017

Okay, none of the moksha APIs are defined or documented and it's a tangled mess of functions accepting "messages" so it's not trivial to say what might break if we suddenly switch from the ZMQMessage class to a plain dict.

My inclination is to either revert the commit that added this and spend some time auditing everything and writing careful tests, or add a check to see if it's a dict before placing headers in the body. @ralphbean, since this is a feature you needed, do you have an opinion one way or the other?

jeremycline added a commit that referenced this pull request Aug 9, 2017

Only add headers if the message is a dict
This is a short-term fix for #462. We don't know what type of object is
being passed as a message.

See #465 for follow-up to
this.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
@codecov-io

This comment has been minimized.

codecov-io commented Aug 9, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #462   +/-   ##
========================================
  Coverage    58.91%   58.91%           
========================================
  Files           29       29           
  Lines         1835     1835           
  Branches       303      303           
========================================
  Hits          1081     1081           
  Misses         667      667           
  Partials        87       87

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 9a35bd4...395bffa. Read the comment docs.

jeremycline added a commit that referenced this pull request Aug 9, 2017

Only add headers if the message is a dict
This is a short-term fix for #462. We don't know what type of object is
being passed as a message.

See #465 for follow-up to
this.

This is a cherry-pick of #462

Signed-off-by: Jeremy Cline <jeremy@jcline.org>

relrod and others added some commits Aug 9, 2017

ZMQMessage isn't iterable, use __dict__
This was flooding logs with traces after we did updates on 2017-08-09. This is the hotfix I came up with, but if it's fixed another way that's fine with me too. Just wanted to get this out there.
Only add headers if the message is a dict
This is a short-term fix for #462. We don't know what type of object is
being passed as a message.

See #465 for follow-up to
this.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>

@jeremycline jeremycline force-pushed the hotfix-zmqmessage branch from 8931ff8 to 395bffa Aug 14, 2017

@ralphbean

This comment has been minimized.

Contributor

ralphbean commented Aug 16, 2017

add a check to see if it's a dict before placing headers in the body.

This seems like the most practical approach for now, no?

@jeremycline jeremycline merged commit eeb0c86 into develop Aug 16, 2017

4 checks passed

codecov/patch Coverage not affected when comparing 9a35bd4...395bffa
Details
codecov/project 58.91% remains the same compared to 9a35bd4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jeremycline jeremycline deleted the hotfix-zmqmessage branch Aug 16, 2017

sijis added a commit to sijis/fedmsg that referenced this pull request Jan 8, 2018

Only add headers if the message is a dict
This is a short-term fix for fedora-infra#462. We don't know what type of object is
being passed as a message.

See fedora-infra#465 for follow-up to
this.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment