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

Add more details to cross-cluster page #20230811 #20231102 #20230804 #6657

Merged

Conversation

AntonEliatra
Copy link
Contributor

Description

Added more details to cross cluster page focusing on bare metal, kubernetes/helm and curl using UI dev tools

Issues Resolved

#20230811 #20231102 #20230804 from list on issue #4314

Checklist

  • [x ] By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and subject to the Developers Certificate of Origin.
    For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@hdhalter
Copy link
Contributor

hdhalter commented Mar 12, 2024

Thanks, @AntonEliatra! Can you please resolve the Vale errors in the file before we send it for technical review?

@hdhalter hdhalter added 2 - In progress Issue/PR: The issue or PR is in progress. and removed 3 - Tech review PR: Tech review in progress labels Mar 12, 2024
@AntonEliatra
Copy link
Contributor Author

@hdhalter all errors have now been resolved, please let me know if there are other changes needed

Copy link
Contributor

@hdhalter hdhalter left a comment

Choose a reason for hiding this comment

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

Thanks, @AntonEliatra! I have a few comments, but will send for a tech review.

@@ -167,6 +167,9 @@ curl -k -XPUT -H 'Content-Type: application/json' -u 'admin:<custom-admin-passwo
}
}'
```
All of the curl requests can also be sent using OpenSearch Dashboards Dev tools
{: .label .label-green }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{: .label .label-green }
{: .tip }

@AntonEliatra - You'll want to use Callouts for this type of info, not Labels.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add another H2 above this section? Something like, "Configuring cross-cluster search from Dashboards"? (I would rethink the other headings, too.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hdhalter I dont think we discussed this today, can you elaborated please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is only one screenshot just to demonstrate that all of the curl commands can be executed from dev tools, Do you mean to add more screenshots and expand on this?

_security/access-control/cross-cluster-search.md Outdated Show resolved Hide resolved
@hdhalter hdhalter added 3 - Tech review PR: Tech review in progress and removed 2 - In progress Issue/PR: The issue or PR is in progress. labels Mar 13, 2024
@hdhalter
Copy link
Contributor

Hi @scrawfor99, can you please review for technical accuracy? Thanks!

Copy link
Contributor

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

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

Thanks @AntonEliatra. Everything written looks correct on review.

I do have a couple general points for you and @hdhalter to consider:

  1. I am not sure why we are splitting the examples into different infrastructure types. Looking at the examples, these seem standard across platforms. I am not sure whether splitting these into supported frameworks would cause a misunderstanding or perception that some of the commands only work on one system. I would expect all of the standard requests and commands to properly work on all systems.

  2. I am not sure whether this page is better served in the search documentation directory. Other than the fact that you will need to authorize and authenticate with the requests (which is true for all of the requests when security is enabled) I don't see any security specific.

@AntonEliatra
Copy link
Contributor Author

@scrawfor99 thanks for above feedback, I do agree that the docker example is quite comprehensive and commands apply to any installation, however there has been quite a few questions as to why its docker specific and how to configure this via UI, we could just state at the very top, that the example is using docker, but the steps/commands apply to any install, but will this sufficiently address the existing questions? @hdhalter what do you think?

Regarding point 2, I do agree, this page seems better placed under "Search" docs, I'm happy enough to change.

@hdhalter
Copy link
Contributor

@peterzhuamazon - Can you please comment on Anton's suggestion above for positioning the Docker instructions?

@stephen-crawford
Copy link
Contributor

Hi @AntonEliatra, gotcha. I definitely don't want to be a pain or anything. I think having the documentation like you wrote it would be just fine. My comment was more for the sake of @Naarcha-AWS or @hdhalter etc. for when they review as something they may consider.

I am in a bit of a unique case since I help with the documentation a lot but don't have as much opinion on the formatting as the technical content etc. Since you nailed the details there I have no issues :)

Looks like Peter may be able to give some advice on where things can best go

@hdhalter hdhalter added 4 - Doc review PR: Doc review in progress and removed 3 - Tech review PR: Tech review in progress labels Mar 26, 2024
Signed-off-by: Heather Halter <[email protected]>
Signed-off-by: Heather Halter <[email protected]>
@hdhalter
Copy link
Contributor

hdhalter commented Mar 27, 2024

Thanks, @AntonEliatra !

@Naarcha-AWS - I made a few more tweaks to the content to add some context and fixed a few formatting issues. Please review at your convenience. Thanks!

_search-plugins/cross-cluster-search.md Outdated Show resolved Hide resolved
_search-plugins/cross-cluster-search.md Outdated Show resolved Hide resolved
_search-plugins/cross-cluster-search.md Outdated Show resolved Hide resolved
@Naarcha-AWS Naarcha-AWS added 5 - Editorial review PR: Editorial review in progress and removed 4 - Doc review PR: Doc review in progress labels Mar 29, 2024
@Naarcha-AWS Naarcha-AWS dismissed hdhalter’s stale review April 3, 2024 15:13

Review feedback implemented.

@AntonEliatra AntonEliatra requested a review from epugh as a code owner April 3, 2024 15:13
Copy link
Collaborator

@natebower natebower left a comment

Choose a reason for hiding this comment

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

@AntonEliatra @Naarcha-AWS Please see my comments and changes and let me know if you have any questions. Thanks!

_search-plugins/cross-cluster-search.md Outdated Show resolved Hide resolved
_search-plugins/cross-cluster-search.md Outdated Show resolved Hide resolved
_search-plugins/cross-cluster-search.md Outdated Show resolved Hide resolved
_search-plugins/cross-cluster-search.md Outdated Show resolved Hide resolved
_search-plugins/cross-cluster-search.md Outdated Show resolved Hide resolved
_search-plugins/cross-cluster-search.md Outdated Show resolved Hide resolved
_search-plugins/cross-cluster-search.md Outdated Show resolved Hide resolved
_search-plugins/cross-cluster-search.md Outdated Show resolved Hide resolved
_search-plugins/cross-cluster-search.md Outdated Show resolved Hide resolved
_search-plugins/cross-cluster-search.md Outdated Show resolved Hide resolved
@Naarcha-AWS Naarcha-AWS added the backport 2.13 PR: Backport label for 2.13 label Apr 5, 2024
@Naarcha-AWS Naarcha-AWS merged commit cc47854 into opensearch-project:main Apr 5, 2024
5 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 5, 2024
…6657)

* adding more details to cross-cluster page #20230811 #20231102 #20230804

Signed-off-by: AntonEliatra <[email protected]>

* adding more details to cross-cluster page #20230811 #20231102 #20230804

Signed-off-by: AntonEliatra <[email protected]>

* adding more details to cross-cluster page #20230811 #20231102 #20230804

Signed-off-by: AntonEliatra <[email protected]>

* adding more details to cross-cluster page #20230811 #20231102 #20230804

Signed-off-by: AntonEliatra <[email protected]>

* adding more details to cross-cluster page #20230811 #20231102 #20230804

Signed-off-by: AntonEliatra <[email protected]>

* adding more details to cross-cluster page #20230811 #20231102 #20230804

Signed-off-by: AntonEliatra <[email protected]>

* moving cross cluster search to new page #20230811 #20231102 #20230804

Signed-off-by: AntonEliatra <[email protected]>

* moving cross cluster search to new page #20230811 #20231102 #20230804

Signed-off-by: AntonEliatra <[email protected]>

* Light edits

Signed-off-by: Heather Halter <[email protected]>

* A few more tweaks

Signed-off-by: Heather Halter <[email protected]>

* Apply suggestions from code review

Signed-off-by: Naarcha-AWS <[email protected]>

* Apply suggestions from code review

Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: AntonEliatra <[email protected]>

* fixes to cross-cluster page #20230811 #20231102 #20230804

Signed-off-by: AntonEliatra <[email protected]>

* fixes to cross-cluster page #20230811 #20231102 #20230804

Signed-off-by: AntonEliatra <[email protected]>

* adding more details to cross-cluster page #20230811 #20231102 #20230804

Signed-off-by: AntonEliatra <[email protected]>

* Update _search-plugins/cross-cluster-search.md

Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: AntonEliatra <[email protected]>

---------

Signed-off-by: AntonEliatra <[email protected]>
Signed-off-by: Heather Halter <[email protected]>
Signed-off-by: Naarcha-AWS <[email protected]>
Co-authored-by: Heather Halter <[email protected]>
Co-authored-by: Naarcha-AWS <[email protected]>
Co-authored-by: Nathan Bower <[email protected]>
(cherry picked from commit cc47854)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Editorial review PR: Editorial review in progress backport 2.13 PR: Backport label for 2.13 security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants