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

v2: All for v2: Exemplars, Cleanup, Docs, Lint, Proto upgrades and more #543

Merged
merged 2 commits into from
Mar 23, 2023

Conversation

bwplotka
Copy link
Collaborator

@bwplotka bwplotka commented Mar 15, 2023

Fixes #347
Fixes #343
Fixes #345
Fixes #483
Fixes #422

Most (if not all) changes for v2.0.0!

Let me know if I need to split into commits for easier review.

TODO:

  • Finish migration guide
  • Finish example README
  • Find out how to change main to v2 without breaking v1 users. All tested https://github.com/golang/go/wiki/Modules#faqs--multi-module-repositories . One finding: We will need to have multiple releases for each submodule in providers (will create quick script). We can start with 0.1.0 for some of them too and probably it makes sense to do so. Changing default branch to "v2" once v2.0.0 is out, should be fine.

@bwplotka
Copy link
Collaborator Author

bwplotka commented Mar 18, 2023

At least one thing still bothers me before v2:

So, interceptors/* have interceptors that sometimes consumes providers. Ironically providers have literally only logging adapters and prometheus interceptor (!). It might feel quite odd. Perhaps moving providers/prometheus to interceptors/prometheus (as a separate module) might make more sense? I don't think we plan to add any other metric middleware so it could be equally interceptors/metrics 🤔 (also avoids package name collision). The only odd thing about this alternative is that "metrics" will be the only interceptor that is a separate module (if we want to NOT pollute go-grpc-middleware with very common prometheus import)... WDYT @johanbrandhorst @devnev ?

EDIT: I think I explained all clearly in README, so I would stick to this. Having separate modules in one directory is quite nice.

@bwplotka bwplotka force-pushed the v2-clean branch 2 times, most recently from f01b009 to 78048fe Compare March 19, 2023 00:47
@bwplotka
Copy link
Collaborator Author

All done and ready for review.

@bwplotka bwplotka changed the title v2: Majority of work towards v2: Exemplars, Cleanup, Docs, Lint, Proto upgrades and more v2: All for v2: Exemplars, Cleanup, Docs, Lint, Proto upgrades and more Mar 19, 2023
Signed-off-by: bwplotka <bwplotka@gmail.com>
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Changing default branch to "v2" once v2.0.0 is out, should be fine.

I would prefer that we make a v1 branch off main and merge v2 into main instead. That's what we did for the gateway.

Do we have dependabot set up? With this module sprawl we'll want something to manage updates.

examples/client/main.go Show resolved Hide resolved
interceptors/auth/examples_test.go Show resolved Hide resolved
interceptors/callmeta.go Outdated Show resolved Hide resolved
interceptors/client_test.go Outdated Show resolved Hide resolved
interceptors/logging/options.go Show resolved Hide resolved
providers/logr/examples_test.go Show resolved Hide resolved
providers/logrus/examples_test.go Show resolved Hide resolved
providers/prometheus/doc.go Show resolved Hide resolved
testing/testpb/interceptor_suite.go Outdated Show resolved Hide resolved
testing/testpb/pingservice_test.go Outdated Show resolved Hide resolved
@bwplotka
Copy link
Collaborator Author

I think I addressed all @johanbrandhorst

Signed-off-by: bwplotka <bwplotka@gmail.com>
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this Bartek 🎉

@bwplotka bwplotka merged commit 85304c0 into v2 Mar 23, 2023
@bwplotka bwplotka deleted the v2-clean branch March 23, 2023 16:57
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

2 participants