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

move one mimirpb custom type to separate package #10316

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

francoposa
Copy link
Member

What this PR does

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I'm missing the context of this change. However, instead of calling the package mimirpb_custom have you considered naming the package based on the actual content, e.g. labelspb?

@francoposa
Copy link
Member Author

I'm missing the context of this change. However, instead of calling the package mimirpb_custom have you considered naming the package based on the actual content, e.g. labelspb?

@pracucci it is not yet fleshed out - it appears making this a draft did not stop it from tagging a lot of people for review.

This PR is just part of an exploration of what it would take to move off of gogoproto - specifically here I am looking at removing the need for the custom type extension and still getting everything to compile & import correctly.

As far as package construction, we can look at specifics when this gets closer to an actual in project, but the mimirpb package is the only which exports custom proto types - specifically this LabelAdapter, PreallocatingMetric, PreallocTimeseries, and UnsafeByteSlice.
I just moved LabelAdapter here to prove out the process, but I figured it would be least disruptive but to keep those together especially as they all reference / build on each other.

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