-
Notifications
You must be signed in to change notification settings - Fork 327
Add build tag to build without deprecated global name validation scheme #804
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Julius Hinze <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
e0ea9ba
to
5470322
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.
Pull Request Overview
The PR introduces a localvalidationscheme
build tag to switch between global and local name/metric/label validation schemes without changing the public API, and updates CI to test both modes.
- Replace uses of the global
NameValidationScheme
with parameterizedvalidate
methods and corresponding build-tag files. - Add
*_localvalidationscheme.go
and*_globalvalidationscheme.go
variants forsilence
,metric
,labelset
,labels
, andalert
. - Update
expfmt
decoder to accept a validation scheme and adjust tests accordingly. - Extend CircleCI matrix to run builds/tests with and without the
localvalidationscheme
tag. - Bump
client_golang
andprocfs
dependencies.
Reviewed Changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
model/silence.go | Refactor Validate to validate(scheme) for local scheme. |
model/silence_globalvalidationscheme.go | Global‐tagged Validate() implementations using global var. |
model/silence_localvalidationscheme.go | Local‐tagged Validate(scheme) that forwards to internal method. |
model/silence_test.go | Use validateMatcher / validateSilence helpers. |
model/silence_globalvalidationscheme_test.go | Global build tests for silence validation. |
model/silence_localvalidationscheme_test.go | Local build tests for silence validation. |
model/metric.go | Rename IsValidMetricName to isValidMetricName(n, scheme) . |
model/metric_globalvalidationscheme.go | Global‐tagged IsValidMetricName(n) using global var. |
model/metric_localvalidationscheme.go | Local‐tagged IsValidMetricName(n, scheme) . |
model/metric_test.go | Delegate to testIsValidMetricName . |
model/metric_globalvalidationscheme_test.go | Global build metric tests. |
model/metric_localvalidationscheme_test.go | Local build metric tests. |
model/labelset.go | Rename Validate /UnmarshalJSON to internal validate(scheme) . |
model/labelset_globalvalidationscheme.go | Global‐tagged Validate() /UnmarshalJSON . |
model/labelset_localvalidationscheme.go | Local‐tagged Validate(scheme) /UnmarshalJSON . |
model/labels.go | Rename IsValid /Unmarshal* to isValid(scheme) and use json/yaml interfaces. |
model/labels_globalvalidationscheme.go | Global‐tagged IsValid() /unmarshal methods. |
model/labels_localvalidationscheme.go | Local‐tagged IsValid(scheme) /unmarshal methods. |
model/alert.go | Rename Validate to validate(scheme) and pass scheme down. |
model/alert_globalvalidationscheme.go | Global‐tagged Validate() . |
model/alert_localvalidationscheme.go | Local‐tagged Validate(scheme) . |
expfmt/decode.go | Remove unconditional NewDecoder; use scheme-aware decoder. |
expfmt/decode_globalvalidationscheme.go | Global‐tagged NewDecoder(r, fmt) and protoDecoder logic. |
expfmt/decode_localvalidationscheme.go | Local‐tagged NewDecoder(r, fmt, scheme) and protoDecoder . |
expfmt/decode_test.go | Adapt tests to use newDecoder with scheme. |
expfmt/decode_globalvalidationscheme_test.go | Global‐tagged decoder tests. |
expfmt/decode_localvalidationscheme_test.go | Local‐tagged decoder tests. |
.circleci/config.yml | Update CI matrix to include localvalidationscheme tag. |
go.mod | Bump client_golang and procfs indirect dependencies. |
Comments suppressed due to low confidence (1)
.circleci/config.yml:100
- The job name interpolation uses
<<# matrix.build_tags >>
, which is not valid CircleCI syntax. It should be<< matrix.build_tags >>
. To avoid an extra dash when the tag is empty, consider using separate job entries or conditional logic in thename
field.
name: go-<< matrix.go_version >><<# matrix.build_tags >>-<< matrix.build_tags >><</matrix.build_tags >>
Signed-off-by: Arve Knudsen <[email protected]>
5ed690a
to
18a45b8
Compare
How long do you think we'll need the two build tags? It can be brittle to have two code paths like this, even if the divergence is small. I know other projects still need time to update their code for the new APIs but we'll want to try to set a deadline. Usually a build tag is exposed somewhere in the Makefile, and that's a good place to mark it experimental. We'll also need README and other doc updates to explain what's going on. |
Signed-off-by: Arve Knudsen <[email protected]>
18a45b8
to
5b1d949
Compare
Signed-off-by: Arve Knudsen <[email protected]>
5b1d949
to
752251b
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.
trying to manage expectations for the future.
Co-authored-by: Owen Williams <[email protected]> Signed-off-by: Arve Knudsen <[email protected]>
Would it be possible to use a new function name for the API rather than rely on a build tag? Since the function signature has changed, all of the call sites in mimir will need to change already anyway, right? |
Add build tag
localvalidationscheme
, to support building without the deprecated global metric/label name validation scheme (model.NameValidationScheme
). This is to support building projects such as Prometheus and Grafana Mimir with a configurable metric/label name validation scheme, without changing the API unless said build tag is used.Since
model.NameValidationScheme
is deprecated, the new build tag should eventually become the default, but I figure we should move in stages.I'm also updating the CircleCI job matrix to exercise building with the new tag.
I'm forced to update the
client_golang
dependency, since it's circular (ideally we'd get rid of this awkward dependency).As agreed with @ywwg - I've documented the new, experimental, build tag in README.md.
See my client_golang draft PR for reference.