Skip to content
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

Updated the label and container port in doc on Setting up Metrics Mon… #1332

Merged
merged 2 commits into from
Jun 25, 2024

Conversation

Fortune-Ndlovu
Copy link
Contributor

@Fortune-Ndlovu Fortune-Ndlovu commented Jun 17, 2024

  • Replaced janus-idp.io/app with rhdh.redhat.com/app
  • Replaced the container port name from http-backend to backend
  • Made sure the instructions provided work correctly

Why the documentation update?
In the ServiceMonitor resource to create, the label added by the Operator had changed from janus-idp.io/app to rhdh.redhat.com/app a long time ago (https://github.com/janus-idp/operator/issues/199) before the first version of the operator was released, and we forgot to update this documentation.

Also, the container port endpoint to scrape metrics from is wrong. It should be backend as per the 1.1.x, 1.2.x, or main branches in the operator repo.

As a consequence, the instructions depicted in this https://github.com/janus-idp/backstage-showcase/blob/main/showcase-docs/monitoring-and-logging.md#operator-backed-deployment will not work as expected.

…itoring for Operator-based deployments

- Change label from janus-idp.io/app to rhdh.redhat.com/app
- Correct container port from http-backend to backend
@Fortune-Ndlovu Fortune-Ndlovu requested a review from a team as a code owner June 17, 2024 13:41
@openshift-ci openshift-ci bot requested review from kadel and nickboldt June 17, 2024 13:41
@rm3l
Copy link
Member

rm3l commented Jun 18, 2024

/cc

@openshift-ci openshift-ci bot requested a review from rm3l June 18, 2024 12:27
Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

Awesome - thanks @Fortune-Ndlovu for fixing this and making sure it works as expected !!

@rm3l
Copy link
Member

rm3l commented Jun 18, 2024

/approve
/override "PR Docker Build"

Doc-only PR

Copy link

openshift-ci bot commented Jun 18, 2024

@rm3l: rm3l unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:.

In response to this:

/approve
/override "PR Docker Build"

Doc-only PR

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-sigs/prow repository.

Copy link

openshift-ci bot commented Jun 18, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rm3l
Once this PR has been reviewed and has the lgtm label, please assign invinciblejai for approval. For more information see the Kubernetes Code Review Process.

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

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

@coreydaley
Copy link
Member

/override "PR Docker Build"
/ok-to-test

Copy link

openshift-ci bot commented Jun 18, 2024

@coreydaley: Overrode contexts on behalf of coreydaley: PR Docker Build

In response to this:

/override "PR Docker Build"
/ok-to-test

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-sigs/prow repository.

@coreydaley
Copy link
Member

/skip ci/prow/images

@rm3l
Copy link
Member

rm3l commented Jun 19, 2024

@coreydaley (or anyone else) Can you /approve this PR so it can be merged? Looks like I don't have permission to do so. I confirm that the change to the docs looks good.

@nickboldt nickboldt merged commit eaf0c4c into janus-idp:main Jun 25, 2024
3 of 4 checks passed
@nickboldt
Copy link
Member

instead of all the comments to disable tests, why not just merge this directly? seems more efficient...

@rm3l
Copy link
Member

rm3l commented Jun 26, 2024

instead of all the comments to disable tests, why not just merge this directly? seems more efficient...

Indeed, I would have merged this simple PR directly, but I don't have permission on this repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants