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

feat: reworked optional data sent in emitted events #140

Merged

Conversation

DuckBoss
Copy link
Contributor

@DuckBoss DuckBoss commented Jul 18, 2023

This commit is proposing a change in how optional data is sent in emitted events
and parsed in the yggdrasil dispatcher to simplify the process if additional optional
data needs to be included in emitted worker events in the future.
This update should also not show any changes in behaviour compared to the main branch.

This work primarily came out of attempts to add a responseTo optional field in the emitted event
while avoiding parsing complications in the dispatcher. [#141]

@DuckBoss DuckBoss force-pushed the jajerome/event-optional-data-improvements branch 2 times, most recently from d709bf4 to 09c110f Compare July 19, 2023 20:36
@DuckBoss DuckBoss force-pushed the jajerome/event-optional-data-improvements branch from 09c110f to a9a3c93 Compare August 11, 2023 17:56
@DuckBoss DuckBoss marked this pull request as ready for review August 14, 2023 20:16
Copy link
Collaborator

@subpop subpop left a comment

Choose a reason for hiding this comment

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

I like this approach. Since it already breaks API compatibility, we might as well update the struct and EmitEvent method to expect map[string]string types instead.

worker/worker.go Outdated
@@ -158,11 +158,14 @@ func (w *Worker) Transmit(addr string, id string, responseTo string, metadata ma
return
}

// EmitEvent emits a WorkerEvent, worker message id, and an optional message.
// EmitEvent emits a WorkerEvent, worker message id, and key-value pairs of optional data.
func (w *Worker) EmitEvent(event ipc.WorkerEventName, messageID string, message string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method signature might as well change to accept a data map[string]string parameter instead of message string.

}
event.Message = eventMessageData["message"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, the WorkerEvent struct should change to have a data map[string]string field.

@DuckBoss DuckBoss force-pushed the jajerome/event-optional-data-improvements branch 2 times, most recently from 32e4520 to ad0bab9 Compare August 25, 2023 14:25
@DuckBoss DuckBoss changed the title [WIP] feat: reworked optional data sent in emitted events feat: reworked optional data sent in emitted events Aug 29, 2023
@DuckBoss DuckBoss requested a review from subpop August 29, 2023 13:12
@DuckBoss DuckBoss force-pushed the jajerome/event-optional-data-improvements branch from 582caa4 to 8dcbe2b Compare August 29, 2023 14:00
Copy link
Collaborator

@subpop subpop left a comment

Choose a reason for hiding this comment

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

I appreciate the usage of golines to wrap long lines into multiple shorter ones. However, the effective change of this PR is difficult to discern due to the extraneous line wraps. Could you please either keep all the formatting changes into a separate commit, or run golines over the entire codebase and open a separate PR updating all the lines in one commit. I think I'd prefer the deliberate PR approach, as it can both keep the changes in a single commit and solve this formatting change for us once. But if you'd prefer a single commit on one PR, I understand.

@subpop
Copy link
Collaborator

subpop commented Sep 1, 2023

Actually, running golines across the whole codebase is easy. I'll do that in a PR now.

@DuckBoss
Copy link
Contributor Author

DuckBoss commented Sep 1, 2023

I appreciate the usage of golines to wrap long lines into multiple shorter ones. However, the effective change of this PR is difficult to discern due to the extraneous line wraps. Could you please either keep all the formatting changes into a separate commit, or run golines over the entire codebase and open a separate PR updating all the lines in one commit. I think I'd prefer the deliberate PR approach, as it can both keep the changes in a single commit and solve this formatting change for us once. But if you'd prefer a single commit on one PR, I understand.

Yeah I had conflicting thoughts about this too, but I agree running it once over the entire codebase would be a better idea. It may be a good idea to update the "contributing" doc to make golines in future PRs mandatory instead of the current optional suggestion and if possible add a golines check to the linting GH action that's currently in use.

@DuckBoss DuckBoss force-pushed the jajerome/event-optional-data-improvements branch from 8dcbe2b to c894be3 Compare September 1, 2023 15:27
@DuckBoss DuckBoss requested a review from subpop September 1, 2023 15:54
Signed-off-by: Jason Jerome <jajerome@redhat.com>
Signed-off-by: Jason Jerome <jajerome@redhat.com>
@DuckBoss DuckBoss force-pushed the jajerome/event-optional-data-improvements branch from c894be3 to 2de113a Compare September 5, 2023 15:45
Copy link
Collaborator

@subpop subpop left a comment

Choose a reason for hiding this comment

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

Looks great!

@subpop subpop merged commit 72726cf into RedHatInsights:main Sep 6, 2023
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.

2 participants