-
Notifications
You must be signed in to change notification settings - Fork 450
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
Remove default claims - fixes #1281 #1282
Remove default claims - fixes #1281 #1282
Conversation
c16b69f
to
c9cb34d
Compare
5e9c755
to
dc86570
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, just one question
@@ -76,11 +76,6 @@ func Container(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelem | |||
|
|||
if len(otelcol.Spec.VolumeMounts) > 0 { | |||
volumeMounts = append(volumeMounts, otelcol.Spec.VolumeMounts...) | |||
} else if otelcol.Spec.Mode == "statefulset" { |
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.
so the users will have to define the volumes explicitly in the CR?
I don't think we have even documented this default volume so not sure somebody used it.
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.
If you need a volume you're need to declare one to have one yes. Basically if not you Will always end up having PVCs / vols required that in some circumstances is not needed by default
thanks, merging this |
Resolves #1281