-
Notifications
You must be signed in to change notification settings - Fork 494
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
Fixed labels and annotations filter #2828
Fixed labels and annotations filter #2828
Conversation
Signed-off-by: Yuri Sa <[email protected]>
Signed-off-by: Yuri Sa <[email protected]>
Signed-off-by: Yuri Sa <[email protected]>
Signed-off-by: Yuri Sa <[email protected]>
Signed-off-by: Yuri Sa <[email protected]>
Signed-off-by: Yuri Sa <[email protected]>
pattern, compileErr := regexp.Compile(basePattern) | ||
match := pattern.MatchString(sourceSet) | ||
if compileErr != nil { | ||
logger.Error(compileErr, "could not compile the regexp pattern") |
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.
is there a logger could pass in as a parameter instead here?
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 don't know if it's a good idea, once we call this function for each annotation/label we put in our resources.
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 all the callers should have a logger on them... but maybe not... i just don't love the random logger here and would like it to use the logger we propagate.
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.
Sounds good. I've changed the function signature to support the logger we propagate.
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.
thank you for doing all that parameter drilling :)
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.
one question, otherwise this is looking pretty good. Do we need to update any e2e tests?
No, I did change the github action passing the proper parameter when we setup the operator's test. |
Signed-off-by: Yuri Sa <[email protected]>
change_type: 'bug_fix' | ||
|
||
# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action) | ||
component: operator |
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.
there is no component operator.
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.
This change is probably for the collector
@@ -18,28 +18,36 @@ import ( | |||
"regexp" | |||
"strings" | |||
|
|||
"github.com/go-logr/logr" |
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.
the API imports in this file are not ordered. the metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
should be here
|
||
"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" | ||
"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" | ||
"github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils" | ||
"github.com/open-telemetry/opentelemetry-operator/internal/naming" | ||
"github.com/open-telemetry/opentelemetry-operator/internal/version" | ||
|
||
"github.com/go-logr/logr" |
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.
these should be above the internal imports
The PR needs to be rebased as well |
I will close this PR because I will submit a new one putting the Annotations/Labels filter during the startup process. |
Description:
Fixing the Labels and Annotations filter.
Link to tracking Issue(s):
Testing:
Documentation: