OSDOCS-34: Updating with the latest examples #14309
Conversation
bergerhoffer
added
peer-review-needed
openshift-ci-robot
added
the
size/M
|
@lilic Could you take a look at these updates, based on the latest from your PR: operator-framework/operator-sdk#719 It looked like the updates were mostly to the code examples, but I made a few other adjustments. Let me know if you have any questions on any of the changes. Thanks! You can preview the file here: http://file.rdu.redhat.com/~ahoffer/2019/OSDOCS-34/applications/operator_sdk/osdk-monitoring-prometheus.html |
| - See the | ||
| link:https://github.com/coreos/prometheus-operator/blob/7a25bf6b6bb2347dacb235659b73bc210117acc7/Documentation/design.md#servicemonitor[Prometheus Operator documentation] | ||
| for more about the ServiceMonitor CRD. | ||
| * See the |
There was a problem hiding this comment.
We are no longer linking to GitHub-hosted content per our user guide
There was a problem hiding this comment.
@sferich888 I've heard that the request to not link to GitHub content came from you. Just wanted to run this one by you (since you made an exception in the CLI content) to make sure this is indeed a blanket statement and if this instance should be removed.
Just want to make sure I understand. Thanks!
There was a problem hiding this comment.
Why would we not link to downstream docs for that topic?
https://github.com/openshift/openshift-docs/blob/enterprise-4.0/monitoring/monitoring.adoc looks like it's missing content if you don't have something to link too
There was a problem hiding this comment.
Hi @sferich888, I'm not sure I understand what you're suggesting. It doesn't look like there is more information about ServiceMonitor in that file (which is here: https://docs.openshift.com/container-platform/4.0/monitoring/monitoring.html).
There was a problem hiding this comment.
After discussing further with @sferich888, I opened https://jira.coreos.com/browse/OSDOCS-394 to cover adding the ServiceMonitor details into the docs, and removing this link altogether. Leaving this link as is for now, and will make that update in a separate PR.
ahardin-rh
added
peer-review-done
|
@bergerhoffer Just one comment from me. Otherwise, this looks good! |
modules/osdk-monitoring-prometheus-servicemonitor-creating.adoc
Outdated
Show resolved
Hide resolved
|
The preview will be availble shortly at: |
| ns := "default" | ||
|
|
||
| // Pass the Service(s) to the helper function, which in turn returns the array of ServiceMonitor objects. | ||
| serviceMonitors, err := metrics.CreateServiceMonitors(restConfig, ns, services) |
There was a problem hiding this comment.
@lilic Do we need to add an instruction for the restConfig argument?
There was a problem hiding this comment.
Yes, that makes sense! This is usually done in the main.go file, so the restConfig refers to https://github.com/operator-framework/operator-sdk-samples/blob/master/memcached-operator/cmd/manager/main.go#L69
There was a problem hiding this comment.
@lilic So should I add the following lines below the "ns := "default" line?
// Get a config to talk to the apiserver
restConfig := config.GetConfig()
There was a problem hiding this comment.
@lilic, @jianzhangbjz I added the above line and updated the PR if you want to take a look. Let me know whether that's right or not, thanks!
There was a problem hiding this comment.
@bergerhoffer Thanks! But, for me, I think we just need to add a comment for the restConfig argument. @lilic What do you suggest?
There was a problem hiding this comment.
The only thing I would change is explicitly point out it's the Kubernetes apiserver config:
// restConfig is used for talking to the Kubernetes apiserver
restConfig := config.GetConfig()There was a problem hiding this comment.
@jianzhangbjz @lilic Thank you both for the reviews, can you take a look at the updates and see if this looks good now?
| ns := "default" | ||
|
|
||
| // Pass the Service(s) to the helper function, which in turn returns the array of ServiceMonitor objects. | ||
| serviceMonitors, err := metrics.CreateServiceMonitors(restConfig, ns, services) |
There was a problem hiding this comment.
The only thing I would change is explicitly point out it's the Kubernetes apiserver config:
// restConfig is used for talking to the Kubernetes apiserver
restConfig := config.GetConfig()
|
LGTM, thanks! |
@openshift/team-documentation For peer review please.