-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[cmd/mdatagen]: Add feature gates support to metadata-schema.yaml #11466
base: main
Are you sure you want to change the base?
[cmd/mdatagen]: Add feature gates support to metadata-schema.yaml #11466
Conversation
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.
Thanks! Left some comments
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.
We are missing the code generation! We need to generate something like this:
opentelemetry-collector/featuregate/README.md
Lines 19 to 25 in 388bae7
var myFeatureGate = featuregate.GlobalRegistry().MustRegister( | |
"namespaced.uniqueIdentifier", | |
featuregate.Stable, | |
featuregate.WithRegisterFromVersion("v0.65.0") | |
featuregate.WithRegisterDescription("A brief description of what the gate controls"), | |
featuregate.WithRegisterReferenceURL("https://github.com/open-telemetry/opentelemetry-collector/issues/6167"), | |
featuregate.WithRegisterToVersion("v0.70.0")) |
7514949
to
05ec7bf
Compare
} | ||
|
||
func (f *featureGate) validate(parser *confmap.Conf) error { | ||
if !parser.IsSet("id") { |
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.
We should also check for stage, that is also a required field
if !versionRegexp.MatchString(fmt.Sprintf("%v", parser.Get("from_version"))) { | ||
err = append(err, fmt.Errorf("invalid character(s) in from_version")) | ||
} | ||
if !versionRegexp.MatchString(fmt.Sprintf("%v", parser.Get("to_version"))) { | ||
err = append(err, fmt.Errorf("invalid character(s) in to_version")) | ||
} | ||
if !referenceURLRegexp.MatchString(fmt.Sprintf("%v", parser.Get("reference_url"))) { | ||
err = append(err, fmt.Errorf("invalid character(s) in reference_url")) | ||
} |
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.
All of these should check first where the field is set, otherwise they are effectively required, aren't they?
For example, instead of:
if !versionRegexp.MatchString(fmt.Sprintf("%v", parser.Get("from_version"))) {
err = append(err, fmt.Errorf("invalid character(s) in from_version"))
}
we should do something similar to this
if parser.IsSet("from_version") && !versionRegexp.MatchString(fmt.Sprintf("%v", parser.Get("from_version"))) {
err = append(err, fmt.Errorf("invalid character(s) in from_version"))
}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11466 +/- ##
==========================================
- Coverage 92.15% 92.03% -0.12%
==========================================
Files 432 433 +1
Lines 20253 20412 +159
==========================================
+ Hits 18664 18787 +123
- Misses 1228 1261 +33
- Partials 361 364 +3 ☔ View full report in Codecov by Sentry. |
Description:
Added feature-gates section to metadata-schema.yaml together with id, description, stage and other required. This information can then be used for code generation, documentation generation and by other consumers of the metadata.yaml file
Link to tracking Issue: #9860