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

Remove WorkMsg interface to avoid unneeded marshalling and unmarshalling #604

Merged
merged 1 commit into from
Jan 2, 2018

Conversation

maleck13
Copy link
Contributor

@maleck13 maleck13 commented Jan 2, 2018

Describe what this PR does and why we need it:
Removes the WorkMsg interface as it only seems to be used to marshal msgs into strings and then the subscribers unmarshal back again. Now that we only have one JobMsg type, this seems unnecessary. This PR also adds missing err checks within the subscribers.
Changes proposed in this pull request

  • Remove WorkMsg interface and replace with JobMsg concrete type
  • Add missing err checks within the subscribers
  • remove unneeded unmarshalling within the subscribers

…halling. Add missing error checks in subscribers
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 2, 2018
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d19a6cc on maleck13:remove-wrkmsg-interface into ** on openshift:master**.

Copy link
Contributor

@rthallisey rthallisey left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

Thank you for this PR!

Method: apb.JobMethodDeprovision,
})
}); err != nil {
log.Errorf("failed to set state after deprovision %#v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

var extCreds *apb.ExtractedCredentials
metrics.ProvisionJobFinished()

log.Debug("Processed provision message from buffer")
// HACK: this seems like a hack, there's probably a better way to
// get the data sent through instead of a string
json.Unmarshal([]byte(msg.Render()), &pmsg)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@shawn-hurley shawn-hurley merged commit e56f16a into openshift:master Jan 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants