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(464): add async actions consumer SDK to act API #34

Merged
merged 1 commit into from
May 28, 2024

Conversation

Hamsajj
Copy link
Contributor

@Hamsajj Hamsajj commented May 27, 2024

Ticket(s)

gatewayd-io/gatewayd#464

Description

Adds a consumer SDK to let plugins consume async actions that has been put into a queue and process them.

Related PRs

Development Checklist

  • I have added a descriptive title to this PR.
  • I have squashed related commits together.
  • I have rebased my branch on top of the latest main branch.
  • I have performed a self-review of my own code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added docstring(s) and type annotations to my code.
  • I have made corresponding changes to the documentation (docs).
  • I have added tests for my changes.

Legal Checklist

mostafa
mostafa previously approved these changes May 28, 2024
Copy link
Member

@mostafa mostafa left a comment

Choose a reason for hiding this comment

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

LGTM! Only two minor comments.

Also, zerolog is the logger for GatewayD and plugins use go-hclog instead, like this one that is referenced here and used like this. Since GatewayD collects logs from plugins and converts them to zerolog via the hclog-adapter, if we use zerolog in the plugins, we won't be able to see the consumer logs in the collected logs.

If the code is going to be used in the plugins as well, we should account for both of these loggers or have WithZeroLogLogger and WithHcLogLogger functions to create the Consumer object.

act/consumer.go Outdated Show resolved Hide resolved
act/consumer.go Outdated Show resolved Hide resolved
@mostafa mostafa dismissed their stale review May 28, 2024 14:30

Logger should be fixed.

@Hamsajj Hamsajj force-pushed the 464-async-queue-consumer branch from 41ade0a to de32383 Compare May 28, 2024 16:27
@Hamsajj Hamsajj requested a review from mostafa May 28, 2024 16:30
@Hamsajj
Copy link
Contributor Author

Hamsajj commented May 28, 2024

As it is not used in gatewayd directly (only used in tests), I removed the support for zerolog and changed it to use hclog.

Copy link
Member

@mostafa mostafa left a comment

Choose a reason for hiding this comment

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

@Hamsajj

I was thinking about a LoggerWrapper that can support multiple loggers under the hood, like the multilogger that is described in this article. This becomes important when we want to implement gatewayd-io/gatewayd#472, but that's out of scope of this ticket, so I'll create another ticket to handle this.

@mostafa mostafa merged commit 8b05bf4 into gatewayd-io:main May 28, 2024
1 check passed
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