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

fix for registering nested message types #23

Merged
merged 2 commits into from Jul 7, 2023
Merged

Conversation

thiagodpf
Copy link
Contributor

@mstoykov Related to this issue.

@CLAassistant
Copy link

CLAassistant commented Jul 6, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

Hi @thiagodpf !

First of all, I'd like to thank you for raising the issue and working on a fix 🙇 Great job!

Regarding the PR, it looks good 👍

However, the one possible improvement could be implementing the stack here instead of using the github.com/golang-collections/collections package (the go implementation is pretty straightforward). The rationale is to reduce the number of libraries we import, improving maintainability.

How does that sound?

@thiagodpf
Copy link
Contributor Author

Hi @olegbespalov !!
I think it's great! I will do this!
Do you see any problems if I bring in the stack implementation from the golang-collections library as-is?
Another point is, is there a file/directory where the stack is better placed or should I include it in the client.go file itself?

grpc/client.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@olegbespalov olegbespalov 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 to me! 🚀

Thank you for your contribution! 🙇

Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

Thanks for the work on this 🙇

@mstoykov mstoykov merged commit 0741976 into grafana:main Jul 7, 2023
10 checks passed
@olegbespalov
Copy link
Collaborator

@thiagodpf just a tiny update on the next steps 👍

We're going to pack some more changes into the xk6-grpc "release" and once it is done, bump the new version in the primary k6's repo. Once that is done, the k6's experimental module will get the fixes.

In parallel, we're going to backport the changes to the legacy k6/net/grpc module. For sure if you want you could do the last part also by yourself, but according to our freshly posted contribution guidelines we don't expect this.

Thanks again! 🚀

@thiagodpf
Copy link
Contributor Author

@olegbespalov thanks for accepting my PR!
So in a few hours I will backport the PR into the legacy module.
Thanks for all the help!

@thiagodpf
Copy link
Contributor Author

Done! ✌️😃
grafana/k6#3178

Again, thanks for the support and help!

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.

None yet

4 participants