-
Notifications
You must be signed in to change notification settings - Fork 437
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
contrib: add support for cloud.google.com/go/pubsub #721
Conversation
f914da7
to
604a120
Compare
Not sure how to fix the build here:
All those symbols exist in recent releases of |
Thanks for doing this. I hope one of us can review it soon (myself or @knusbaum). For the future, please make sure to read our contribution guidelines first. I wouldn't want us to end up in the uncomfortable situation where we would have to reject a PR into which someone has put a lot of time and effort. It many times helps figure out how to approach things by discussing them first. Nevertheless, your contribution is very welcome and new integrations are always nice to have! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how to fix the build here
What's the problem? Why is it working for you locally and not in the CI? Are you using different versions?
- Please use the right folder name in the
contrib
folder and include the version (check this out) - Add an
example_test.go
file - This is not an actual review, just a few pointers.
As mentioned above, I'm not really sure of what's wrong. Locally I'm using the most recent version (1.6.1) and that is also included in the Normally to fix this I would add a constraint in
Done.
Done.
Standing by. |
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@datadoghq.com>
e1b629e
to
042478a
Compare
OK, I found the problem - it seems you're using the circleci config file as a substitute for go.mod for contrib. I wasn't really expecting that, so didn't think of checking there. Should work now. |
ae185d0
to
b29a8c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken some time to look deeper into the CI problem. I left some comments. I hope it helps. Let me know what you think...
P.S. I hope you'll be patient with our many rounds of reviews :)
I've allowed myself to push that change (#721 (comment)) to your branch. It seems to have worked. I hope that's ok with you. |
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@datadoghq.com>
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@datadoghq.com>
Don't mind at all! Any help is greatly appreciated. |
395ebe3
to
c09b1ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you once again for making even more changes and having patience.
I hope you are ok with my many rounds of reviews and they aren't getting annoying. I think that being very careful with code and what gets merged makes a big difference on the long run and code quality is something very important to me and this project. I want us all to be as happy as possible with the work.
I realise that maybe some comments could've been made earlier too but I prefer to start by first reviewing conventions/style/structure etc and only after that's sorted out looking at the actual functionality, which is what this review is.
I feel we are very close and am quite happy with the work.
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@datadoghq.com>
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@datadoghq.com>
OK, all done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I'm happy with the result, and I hope you are too!
I might push a few more commits to this change to better understand why CI is failing without the hack and then merge eventually.
Thanks once more. You should know that I've taken your feedback to heart and as we'll see those problems arising we should consider changing our ways. @knusbaum PTAL at Carlo's thoughts in #721 (comment) |
Add GCP cloud pubsub tracing helpers.
Unfortunately the official cloud pubsub library implements a very simple
struct
-based design, so it's hard to build a composable middleware for it.Closes #722