-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add management support for the multicluster-observability-operator #53
Conversation
Pull Request Test Coverage Report for Build 9464617973Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
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.
Before merging this PR, we need to address some of the comments/questions/concerns that were made in the proposal.
Also, if I understood well, there is a meeting soon that can change the proposal.
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 for putting this together and moving from the proposal. I would like to clarify the AddOnDeploymentConfig
and how multiple options of the same capability are supported.
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.
Overall lgtm
ab579cb
to
6647b90
Compare
The use of multiple values in an customizedVariables:
- name: userWorkloadLogsCollection
value: clusterlogforwarders.v1.logging.openshift.io;opentelemetrycollectors.v1beta1.opentelemetry.io translates in the case KeyUserWorkloadLogsCollection:
if keyvalue.Value == string(ClusterLogForwarderV1) {
opts.UserWorkloads.Enabled = true
opts.UserWorkloads.Logs.CollectionEnabled = true
opts.UserWorkloads.Logs.Kinds = append(opts.UserWorkloads.Logs.Kinds, ClusterLogForwarderV1)
}
if keyvalue.Value == string(OpenTelemetryCollectorV1beta1) {
opts.UserWorkloads.Enabled = true
opts.UserWorkloads.Logs.CollectionEnabled = true
opts.UserWorkloads.Logs.Kinds = append(opts.UserWorkloads.Logs.Kinds, OpenTelemetryCollectorV1beta1)
} Note: This is just pseudo-code and not meant to be implemented in this PR yet. I am leaving the room open for you guys to add support for multiple values when you are ready with each signal. WDYT? |
b60119c
to
f0b4815
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.
LGTM (after we clarified
The following PR prepares the addon to be managed by the multicluster-observability-operator. Two major changes are implemented:
AddonDeploymentConfig
.open-cluster-management-observability
Furthermore the proposed implementation adds validation support in the logging handler for:
ClusterLogForwarder
pipeline includesinfrastructure
and/oraudit
input reference whenplatformLogsCollection
set.ClusterLogForwarder
pipeline includesapplication
input reference whenuserWorkloadLogsCollection
set.The above implementations are not comprehensive but illustrative on how the capabilities defined in the
AddonDeploymentConfig
can be used within the addon.