-
Notifications
You must be signed in to change notification settings - Fork 6
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
Adds obsv samples for CSI Powermax #299
Conversation
3824528
to
52f8828
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.
Need some e2e tests for PowerMax + Observability -- write similar tests to what we have for PowerFlex + Obs and PowerScale + Obs
Add cert-manager to yamls as per this PR: #310
If you need any help or want to talk through any of this, hmu on slack
52f8828
to
7eed7a3
Compare
08d4336
to
3b6549f
Compare
Added. |
3b6549f
to
39e5ea1
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.
pkg/modules/observability.go
Outdated
YamlString = string(buf) | ||
|
||
otelCollectorAddress := "otel-collector:55680" | ||
pmaxImage := "dellemc/csm-metrics-powermax:v1.1.0" |
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.
let's make this an empty string instead of a default image value for better error handling
Other than my one comment everything looks good, if you can address it by 6:00 AM EST/3:30 PM IST tomorrow (August 31st) I can approve |
04b3258
to
f0363b4
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.
f0363b4
to
f23d2fc
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.
Adds unit test for obs for powermax Adds e2e for powermax
f23d2fc
to
5a130a1
Compare
It looks operatorconfig/moduleconfig/common/version-values.yaml is not updated for PowerMax + Obs. +CC @donatwork |
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 versions need updating.
- name: observability | ||
# enabled: Enable/Disable observability | ||
enabled: false | ||
configVersion: v1.5.0 |
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.
Should be v1.6.0.
enabled: false | ||
# image: Defines karavi-topology image. This shouldn't be changed | ||
# Allowed values: string | ||
image: dellemc/csm-topology:v1.5.0 |
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.
Shoudl be v1.6.0.
Adds unit test for obsv and e2e for powermax
Description
GitHub Issues
List the GitHub issues impacted by this PR:
Checklist:
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration