-
Notifications
You must be signed in to change notification settings - Fork 208
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
refactor: instrumentation libraries config in CRD #1477
Conversation
|
||
// Configuration boolean options which are enabled for this instrumentation library. | ||
// All options are disabled by default. To enable an option, add it's name to this list. | ||
EnabledOptions []string `json:"enabledOptions,omitempty"` |
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 think that a string alone for an option might be too specific, we can have specific options in the future which might have an integer value for them.
An option can have a string key and an optional string value - WDYT?
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 think in the future, we will have options with various types: numbers, strings, string arrays, etc.
Since they have different types, I want to have different properties in the CRD for each type.
For boolean options, I was thinking we only need to have them if they are true
so the false
value is the default.
That is why I choose to write it this way and postpone the other types for the future.
But perhaps it's better to have the final version here so that we don't need to make other changes down the road
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.
made it better with support for multiple types
// additional attributes to record original length, etc. | ||
type InstrumentationLibraryCapabilityParameter struct { | ||
// The name of the property that should be configured for the instrumentation library option | ||
ParameterName string `json:"parameterName"` |
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.
What is the difference between this and CapabilityName
?
If possible, adding an example in the comments will make it clearer
replaced with #1488 |
No description provided.