Skip to content
This repository was archived by the owner on Jul 10, 2020. It is now read-only.

Document how to setup prometheus-operator for Knative#52

Merged
knative-prow-robot merged 3 commits into
knative:masterfrom
marcobebway:prometheus-operator
Oct 8, 2019
Merged

Document how to setup prometheus-operator for Knative#52
knative-prow-robot merged 3 commits into
knative:masterfrom
marcobebway:prometheus-operator

Conversation

@marcobebway
Copy link
Copy Markdown
Contributor

@marcobebway marcobebway commented Oct 1, 2019

Description

Add documentation guidelines explaining how to setup Prometheus operator for Knative observability.

Resolves knative/eventing#1925

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 1, 2019
@knative-prow-robot
Copy link
Copy Markdown

knative-prow-robot commented Oct 1, 2019

Hi @marcobebway. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-prow-robot knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 1, 2019
@marcobebway marcobebway mentioned this pull request Oct 1, 2019
Closed
wfernandes
wfernandes reviewed Oct 1, 2019
View reviewed changes
Comment thread docs/prometheus-operator/README.md
Comment on lines +230 to +232
```bash
kubectl get pods -n knative-monitoring --selector='app.kubernetes.io/component=controller,app.kubernetes.io/name=prometheus-operator' -w
```
Copy link
Copy Markdown
Contributor

@wfernandes wfernandes Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest expanding on the flag names. Verbosity in docs is fine. Since you have --selector, I suggest using --watch and --namespace.

Copy link
Copy Markdown
Contributor Author

@marcobebway marcobebway Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

wfernandes
wfernandes reviewed Oct 1, 2019
View reviewed changes
Comment thread docs/prometheus-operator/README.md
Comment on lines +5 to +10
## Cluster setup

Create a kubernetes cluster as mentioned [here](https://knative.dev/docs/install/knative-with-gke/#creating-a-kubernetes-cluster).

> Note: A GKE cluster is used, but any other cluster type can be used as well.

Copy link
Copy Markdown
Contributor

@wfernandes wfernandes Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the docs pointed here also contain instructions on installing knative, I feel it may be a little confusing. Maybe provide the simplest command to creating a GKE cluster and providing the cluster admin instructions.

Copy link
Copy Markdown
Contributor Author

@marcobebway marcobebway Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

wfernandes
wfernandes reviewed Oct 1, 2019
View reviewed changes
Comment thread docs/prometheus-operator/README.md Outdated

## Knative setup

### Install Knative [Serving](https://github.com/knative/serving/blob/master/DEVELOPMENT.md#deploy-knative-serving) and [Eventing](https://github.com/knative/eventing/blob/master/DEVELOPMENT.md#starting-eventing-controller).
Copy link
Copy Markdown
Contributor

@wfernandes wfernandes Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you want the users to install the monitoring components of knative/serving as well? To avoid confusion, you can suggest skipping that part.

Copy link
Copy Markdown
Contributor Author

@marcobebway marcobebway Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

wfernandes
wfernandes reviewed Oct 1, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@wfernandes wfernandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a few comments. Just being nit-picky but otherwise looks good to me. Let me know if you have any questions.

@wfernandes
Copy link
Copy Markdown
Contributor

wfernandes commented Oct 1, 2019

/ok-to-test

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 1, 2019
@marcobebway
Fix the README file
ab1784c 
 - Expand the kubectl flag names
 - Remove the trailing punctuation in the headers
 - Add simple commands to create a GKE cluster and to grant the cluster admin permissions
 - Add a note to skip installing the monitoring components coming with Knative serving
@marcobebway marcobebway requested a review from wfernandes October 7, 2019 12:05
@marcobebway
Copy link
Copy Markdown
Contributor Author

marcobebway commented Oct 7, 2019

/assign wfernandes

@wfernandes
Copy link
Copy Markdown
Contributor

wfernandes commented Oct 7, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2019
@marcobebway marcobebway mentioned this pull request Oct 7, 2019
Closed
@wfernandes
Copy link
Copy Markdown
Contributor

wfernandes commented Oct 8, 2019

/approved

@wfernandes
Copy link
Copy Markdown
Contributor

wfernandes commented Oct 8, 2019

@Benjamintf1 Can you mark this as approved?

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2019
@wfernandes
Copy link
Copy Markdown
Contributor

wfernandes commented Oct 8, 2019

/approve

@knative-prow-robot
Copy link
Copy Markdown

knative-prow-robot commented Oct 8, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marcobebway, wfernandes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit acaaeaa into knative:master Oct 8, 2019
@nachocano nachocano mentioned this pull request Oct 8, 2019
Closed
75 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Benjamintf1 Benjamintf1 Awaiting requested review from Benjamintf1

@wfernandes wfernandes Awaiting requested review from wfernandes

Assignees

@wfernandes wfernandes

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add Prometheus Operator

4 participants

@marcobebway @knative-prow-robot @wfernandes @googlebot