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

OCPBUGS#24683: Added Role and RoleBinding objects to enable monitoring #69893

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

snarayan-redhat
Copy link
Contributor

@snarayan-redhat snarayan-redhat commented Jan 8, 2024

@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 8, 2024
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Jan 8, 2024

🤖 Thu Jan 18 12:31:35 - Prow CI generated the docs preview: https://69893--ocpdocs-pr.netlify.app

@snarayan-redhat snarayan-redhat changed the title OCPBUGS#24683: Added Role and RoleBinding objects OCPBUGS#24683: Added Role and RoleBinding objects to enable monitoring Jan 8, 2024
@swghosh
Copy link
Member

swghosh commented Jan 8, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2024
Copy link
Member

@lunarwhite lunarwhite left a comment

Choose a reason for hiding this comment

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

Others LGTM, thanks!

modules/cert-manager-enable-metrics.adoc Outdated Show resolved Hide resolved
modules/cert-manager-enable-metrics.adoc Outdated Show resolved Hide resolved
modules/cert-manager-enable-metrics.adoc Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2024
@lunarwhite
Copy link
Member

/lgtm

@xingxingxia
Copy link
Contributor

/lgtm cancel
@snarayan-redhat as per bug comment but not add and execute the step "Enable monitoring for user-defined projects. See Enabling monitoring for user-defined projects for instructions", the Enable monitoring for user-defined projects. See Enabling monitoring for user-defined projects for instructions is not needed.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2024
@snarayan-redhat
Copy link
Contributor Author

snarayan-redhat commented Jan 12, 2024

/lgtm cancel @snarayan-redhat as per bug comment but not add and execute the step "Enable monitoring for user-defined projects. See Enabling monitoring for user-defined projects for instructions", the Enable monitoring for user-defined projects. See Enabling monitoring for user-defined projects for instructions is not needed.

I had discussed this with @swghosh. He confirmed that we would need this step. @swghosh any inputs?

@swghosh
Copy link
Member

swghosh commented Jan 12, 2024

@xingxingxia Would you prefer both scenarios of monitoring

  • steps to setup monitoring for user-defined workloads
  • steps to setup monitoring of cluster components

During discussion with @snarayan-redhat, it came up that it is desirable to document the most useful use case and went ahead that cert-manager would likely be most suitable as a in-cluster component than a user workload, hence this change was proposed with this behaviour.

@xingxingxia
Copy link
Contributor

went ahead that cert-manager would likely be most suitable as a in-cluster component than a user workload, hence this change was proposed with this behaviour.

@swghosh my previous comment #69893 (comment) is exactly as same as your thought, i.e. remove the "steps to setup monitoring for user-defined workloads" which is documented as "Enable monitoring for user-defined projects. See Enabling monitoring for user-defined projects for instructions" and Additional resources "Enabling monitoring for user-defined projects". It is not yet removed.

@snarayan-redhat
Copy link
Contributor Author

@xingxingxia Would you prefer both scenarios of monitoring

  • steps to setup monitoring for user-defined workloads
  • steps to setup monitoring of cluster components

During discussion with @snarayan-redhat, it came up that it is desirable to document the most useful use case and went ahead that cert-manager would likely be most suitable as a in-cluster component than a user workload, hence this change was proposed with this behaviour.

@swghosh IIUC, @xingxingxia correct me if I am wrong, for this documented scenario, the bug suggested that we need not go for "Enabling monitoring for user-defined projects" in case we are retaining the namespace and adding Role and RoleBinding objects. However, as per our discussion, you suggested that only addition of the objects is required and need not remove Step 2.
For user-defined workloads, we would be opting to remove the namespace.

@lunarwhite
Copy link
Member

Hi @snarayan-redhat, could you also remove the first link("Enabling monitoring for user-defined projects") in "Additional resources" part?

Screenshot 2024-01-16 17 21 11

@snarayan-redhat
Copy link
Contributor Author

Hi @snarayan-redhat, could you also remove the first link("Enabling monitoring for user-defined projects") in "Additional resources" part?

Screenshot 2024-01-16 17 21 11

Done

@lunarwhite
Copy link
Member

/lgtm

Thanks!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2024
@snarayan-redhat snarayan-redhat added the peer-review-needed Signifies that the peer review team needs to review this PR label Jan 17, 2024
@xingxingxia
Copy link
Contributor

LGTM, thx!

@kcarmichael08 kcarmichael08 added peer-review-in-progress Signifies that the peer review team is reviewing this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Jan 17, 2024
Copy link
Contributor

@kcarmichael08 kcarmichael08 left a comment

Choose a reason for hiding this comment

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

One small suggestion; otherwise, LGTM!

modules/cert-manager-enable-metrics.adoc Outdated Show resolved Hide resolved
modules/cert-manager-enable-metrics.adoc Outdated Show resolved Hide resolved
@kcarmichael08 kcarmichael08 added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR labels Jan 17, 2024
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2024
Copy link

openshift-ci bot commented Jan 18, 2024

New changes are detected. LGTM label has been removed.

Copy link

openshift-ci bot commented Jan 18, 2024

@snarayan-redhat: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@snarayan-redhat snarayan-redhat merged commit 40fa290 into openshift:main Jan 18, 2024
2 checks passed
@snarayan-redhat
Copy link
Contributor Author

/cherrypick enterprise-4.12

@openshift-cherrypick-robot

@snarayan-redhat: new pull request created: #70476

In response to this:

/cherrypick enterprise-4.12

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.

@snarayan-redhat
Copy link
Contributor Author

/cherrypick enterprise-4.13

@snarayan-redhat
Copy link
Contributor Author

/cherrypick enterprise-4.14

@snarayan-redhat
Copy link
Contributor Author

/cherrypick enterprise-4.15

@openshift-cherrypick-robot

@snarayan-redhat: new pull request created: #70477

In response to this:

/cherrypick enterprise-4.13

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.

@openshift-cherrypick-robot

@snarayan-redhat: new pull request created: #70478

In response to this:

/cherrypick enterprise-4.14

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.

@openshift-cherrypick-robot

@snarayan-redhat: new pull request created: #70479

In response to this:

/cherrypick enterprise-4.15

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.12 branch/enterprise-4.13 branch/enterprise-4.14 branch/enterprise-4.15 peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants