Skip to content

Fix typo in the cluster logging templates#29637

Merged
codyhoag merged 1 commit intoopenshift:masterfrom
lbscorpio:docs-4.6
Mar 8, 2021
Merged

Fix typo in the cluster logging templates#29637
codyhoag merged 1 commit intoopenshift:masterfrom
lbscorpio:docs-4.6

Conversation

@lbscorpio
Copy link
Copy Markdown
Contributor

@vikram-redhat There are two issues on the templates of cluster logging docs, minimum version that the change applies to should be 4.6 in my opinion, could you review them? Thanks.

  1. modules/cluster-logging-deploy-console.adoc
    One more same comment for <6>, should remove it.

  2. modules/cluster-logging-elasticsearch-audit.adoc
    On OCP 4.6.x(not test on OCP 4.7 yet, might be same):
    spec.outputs.url in body should match '^$|[a-zA-z]+://.*'" for field "spec.outputs.url".
    supported values: "syslog", "fluentdForward", "elasticsearch", "kafka"" for field "spec.outputs.type".

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 21, 2021
@netlify
Copy link
Copy Markdown

netlify Bot commented Feb 21, 2021

Deploy preview for osdocs ready!

Built with commit 1445741

https://deploy-preview-29637--osdocs.netlify.app

@vikram-redhat
Copy link
Copy Markdown
Contributor

@rolfedh can you take a look? Should this apply to 4.7 as well?

@rolfedh
Copy link
Copy Markdown
Contributor

rolfedh commented Feb 21, 2021

@rolfedh can you take a look? Should this apply to 4.7 as well?

Yes, it should apply to 4.7.
I'll ask a Logging QE to review and approve this proposed change before we merge it.

@rolfedh
Copy link
Copy Markdown
Contributor

rolfedh commented Feb 21, 2021

Hello @anpingli. Would you review the issue @lbscorpio describes above and the changes they are proposing? Thank you.

Copy link
Copy Markdown

@anpingli anpingli left a comment

Choose a reason for hiding this comment

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

@lbscorpio
Copy link
Copy Markdown
Contributor Author

@anpingli If so, 4.5 also used the wrong URLs? And http://elasticsearch-secure.svc.messaging.cluster.local should be http://elasticsearch-secure.messaging.svc.cluster.local, am I right? Thanks.

https://docs.openshift.com/container-platform/4.5/logging/cluster-logging-external.html
https://docs.openshift.com/container-platform/4.5/logging/config/cluster-logging-log-store.html

@anpingli
Copy link
Copy Markdown

anpingli commented Feb 22, 2021

@lbscorpio yes, the URLs is wrong before.

@rolfedh
Copy link
Copy Markdown
Contributor

rolfedh commented Feb 22, 2021

@lbscorpio It looks like 4.5 has the same issues you and @anpingli found here, but in slightly different file locations. Therefore, cherrypicking this PR back to 4.5 won't work.

Please let me know if you want me to fix these issues in 4.5, or if you plan to do it.

Rolfe

@lbscorpio
Copy link
Copy Markdown
Contributor Author

lbscorpio commented Feb 22, 2021

@anpingli Sorry, what did you mean? Which URLs not wrong? If the correct one is end with .svc.cluster.local, do you want me to add new change to this PR? Thanks.

https://docs.openshift.com/container-platform/4.5/logging/cluster-logging-external.html
https://docs.openshift.com/container-platform/4.5/logging/config/cluster-logging-log-store.html

Both pages used below URL:
endpoint: elasticsearch-insecure.svc.messaging.cluster.local

@lbscorpio
Copy link
Copy Markdown
Contributor Author

@rolfedh Thanks for your feedback. I will wait @anpingli to double confirm firstly. This PR will handle 4.6+, I can raise another PR for 4.5 once confirm, thanks.

@anpingli
Copy link
Copy Markdown

@lbscorpio Yes, the correct one is end with .svc.cluster.local.

@lbscorpio
Copy link
Copy Markdown
Contributor Author

lbscorpio commented Feb 23, 2021

@anpingli Thanks for your information.

@rolfedh I have added the new change for 4.6, new commit is efb6727. For 4.5, I raised #29679 to correct the URLs, fyi, thanks.

@rolfedh
Copy link
Copy Markdown
Contributor

rolfedh commented Mar 6, 2021

Thanks, @lbscorpio. This looks good. Would you like help to resolve these conflicts?
The duplicate entry in modules/cluster-logging-deploy-console.adoc has already been fixed in master.
Removing that change from your PR should resolve the conflict.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2021
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2021
@lbscorpio
Copy link
Copy Markdown
Contributor Author

@rolfedh I have removed the duplicate entry in modules/cluster-logging-deploy-console.adoc in my commit, new commit is 1445741, thanks.

@rolfedh
Copy link
Copy Markdown
Contributor

rolfedh commented Mar 8, 2021

[enterprise-4.6]
[enterprise-4.7]
[enterprise-4.8]

Copy link
Copy Markdown

@anpingli anpingli left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2021
Copy link
Copy Markdown

@anpingli anpingli left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Copy Markdown
Contributor

@codyhoag codyhoag left a comment

Choose a reason for hiding this comment

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

YAML edits LGTM!

@codyhoag codyhoag added the peer-review-done Signifies that the peer review team has reviewed this PR label Mar 8, 2021
@codyhoag codyhoag changed the title [enterprise-4.6]Fix typo in the cluster logging templates Fix typo in the cluster logging templates Mar 8, 2021
@codyhoag codyhoag merged commit 6722367 into openshift:master Mar 8, 2021
@codyhoag
Copy link
Copy Markdown
Contributor

codyhoag commented Mar 8, 2021

/cherrypick enterprise-4.8

@openshift-cherrypick-robot
Copy link
Copy Markdown

@codyhoag: new pull request created: #30183

Details

In response to this:

/cherrypick enterprise-4.8

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.

@codyhoag
Copy link
Copy Markdown
Contributor

codyhoag commented Mar 8, 2021

/cherrypick enterprise-4.7

@openshift-cherrypick-robot
Copy link
Copy Markdown

@codyhoag: new pull request created: #30184

Details

In response to this:

/cherrypick enterprise-4.7

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.

@codyhoag
Copy link
Copy Markdown
Contributor

codyhoag commented Mar 8, 2021

/cherrypick enterprise-4.6

@openshift-cherrypick-robot
Copy link
Copy Markdown

@codyhoag: new pull request created: #30185

Details

In response to this:

/cherrypick enterprise-4.6

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.

@lbscorpio lbscorpio deleted the docs-4.6 branch March 9, 2021 01:07
@lbscorpio lbscorpio restored the docs-4.6 branch March 9, 2021 01:36
@lbscorpio lbscorpio deleted the docs-4.6 branch March 9, 2021 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.6 branch/enterprise-4.7 branch/enterprise-4.8 lgtm Indicates that a PR is ready to be merged. peer-review-done Signifies that the peer review team has reviewed this PR size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants