chore: bump helm and observabiilty versions #201
Conversation
There was a problem hiding this comment.
Left some comments around the version defaults. It seems like it would be a big win to force users to supply a version (even if they just supply the inbuilt example) declaratively.
Additionally, this would hopefully cut down on the number of areas that have to be manually synced up when bumping versions.
My other question is about the large changes to the OTEL operator config. Are these a necessary part of the version bump? If not perhaps a separate PR with more description might be helpful.
| @@ -40,7 +40,7 @@ def add_namespace(obj): | |||
| chart_name = 'cert-manager' | |||
| chart_version = config.get('chart_version') | |||
| if not chart_version: | |||
| chart_version = 'v1.9.1' | |||
| chart_version = 'v1.10.0' | |||
There was a problem hiding this comment.
Not necessary to adjust in this PR, but in my opinion we should check that the user has specified versions up front and crash with a helpful error message before anything is created. Defaults help with the "plug and play" ability of the example but I think we can have the defaults encoded in the .example as above and not hidden in a python script.
Thoughts on this? Do you think it would be tough to do?
There was a problem hiding this comment.
If we crashed here, would we wind up with any half-created state?
There was a problem hiding this comment.
Definitely, if we crash out things are really pear-shaped.
The long-term plan - and I think there may be an issue with it; if not I will add one - is to remove the fallbacks in the code for versions and force them all through the config. This requires that we make sure we tell the users how to add these values, and we probably should provide a "just give me all the known working values" option.
There was a problem hiding this comment.
Sounds like we are in agreement but we don't need to hold up this chore PR on it. I'll call this another notch to add to #140
| @@ -31,7 +31,7 @@ | |||
| nginx_repository = "nginx/nginx-ingress" | |||
| nginx_tag = config.get('nginx_tag') | |||
| if not nginx_tag: | |||
| nginx_tag = "2.2.0" | |||
| nginx_tag = "2.4.0" | |||
There was a problem hiding this comment.
What does this version number correlate to? I don't see it in the version numbers modified in the .example above.
There was a problem hiding this comment.
There are two paths for the NGINX IC deployment; one is in the mainline where we do everything in Python - this is the "just pull from the repo w/ the JWT" logic, and for that we need the nginx image tag (latest is 2.4.0). If you look at the NGINX version that's the most recent.
There was a problem hiding this comment.
Thanks for the clarification. Ideally this lives in config somewhere but let's put that on #140
| @@ -61,6 +61,131 @@ spec: | |||
| description: InstrumentationSpec defines the desired state of OpenTelemetry | |||
| SDK and instrumentation. | |||
| properties: | |||
| dotnet: | |||
There was a problem hiding this comment.
What is the relation of this change to the version bumps that are the meat of this PR? Is this extra configuration required by a new version of one of the libraries?
There was a problem hiding this comment.
This is strictly a copy of the yaml manifest for OTEL - https://opentelemetry.io/docs/k8s-operator/
Why do we include it here instead of just reaching out to pull it down each time? Paranoia....I don't want this to change and blow up MARA for someone. I want to control the versions, especially since cert-manager and OTEL seem to be super touchy about each other.
| @@ -61,6 +61,131 @@ spec: | |||
| description: InstrumentationSpec defines the desired state of OpenTelemetry | |||
| SDK and instrumentation. | |||
| properties: | |||
| dotnet: | |||
| description: DotNet defines configuration for DotNet auto-instrumentation. | |||
There was a problem hiding this comment.
What is DotNet in this context? Should the description here link to some project page so the reader can find the docs and understand what is being configured here?
There was a problem hiding this comment.
I will put in a link to the OTEL operator repo in the source to folks can track back to it if they need to.
| @@ -289,6 +421,7 @@ spec: | |||
| required: | |||
| - key | |||
| type: object | |||
| x-kubernetes-map-type: atomic | |||
There was a problem hiding this comment.
For these things, do we want to add some link or comment to help users understand what this is? I found this https://kubernetes.io/docs/reference/using-api/_print/#merge-strategy
| TargetAllocator. This should only be set to a value other than | ||
| 1 if a strategy that allows for high availability is chosen. | ||
| Currently, the only allocation strategy that can be run in a | ||
| high availability mode is consistent-hashing. |
There was a problem hiding this comment.
This is a great comment that helps me understand how to set the value 🎉
Proposed changes
Bumping the versions of our helm charts and our observability operator.
Checklist
Before creating a PR, run through this checklist and mark each as complete.