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.

Sign up for GitHub

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

Jump to bottom

Add documentation for building Logstash custom image with opensearch plugin #245

Merged
merged 1 commit into from
Apr 13, 2024
Merged

Add documentation for building Logstash custom image with opensearch plugin #245

merged 1 commit into from
Apr 13, 2024

Conversation

FloMedja
Copy link
Contributor

@FloMedja FloMedja commented Apr 10, 2024 •
edited
Loading

Description

This MR add a documentation to build Logstash custom image with opensearch plugin/

Issues Resolved

Follow-up to this question #209

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has documentation added
  • Commits are signed as per the DCO using --signoff

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

@FloMedja FloMedja mentioned this pull request Apr 10, 2024
Closed
dblock
dblock requested changes Apr 11, 2024
View reviewed changes
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

It's a good start, but it's confusing. Do I have to create the Dockerfile first, but then there are two Dockerfiles in the example? Try to really turn it into a series of steps that one can copy-paste exactly.

  • some nits

README.md Outdated
@@ -28,6 +28,7 @@ The logstash-output-opensearch plugin helps to ship events from Logstash to Open
* [Release Management](RELEASING.md)
* [Admin Responsibilities](ADMINS.md)
* [Security](SECURITY.md)
* [Build custom logstash image](docs/build_custom_logstash_image.md)
Copy link
Member

Choose a reason for hiding this comment

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

Should we link it from https://github.com/opensearch-project/logstash-output-opensearch/blob/main/DEVELOPER_GUIDE.md instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question... Having a section in the developer_guide with a link to the documentation sounds like a good approach to me. @oeyh, what are your thoughts on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

docs/build_custom_logstash_image.md Outdated
@@ -0,0 +1,62 @@
# Build A Custom Image with Logstash
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize/verbalize consistently, maybe "Building a Custom Docker Image with Logstash"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

docs/build_custom_logstash_image.md Outdated
@@ -0,0 +1,62 @@
# Build A Custom Image with Logstash

If you are looking for a logstash image version with opensearch plugin not available in the [official Docker repository tags](https://hub.docker.com/r/opensearchproject/logstash-oss-with-opensearch-output-plugin/tags) or if you want to include other plugins in your image, it is possible to build a custom image.
Copy link
Member

Choose a reason for hiding this comment

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

Add a comma before ", or".

You could say it shorter.

"To build an image that is not available ... or does not include ...", but i'm OCD ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

docs/build_custom_logstash_image.md Outdated

### Building logstash output opensearch gem plugin

1. Clone `logstash-output-opensearch repo` for the plugin version we want to build. For exemple , clone the [v2.0.2](https://github.com/opensearch-project/logstash-output-opensearch/tree/2.0.2)
Copy link
Member

Choose a reason for hiding this comment

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

Fix spelling, "example", remove extra space before ", "

Copy link
Member

Choose a reason for hiding this comment

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

Add an actual command one can run

git clone ...
cd .....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

docs/build_custom_logstash_image.md Outdated

## Logstash 8.x

Below is an example of a `Dockerfile` to install `Opensearch 2.0.2` for **`Logstash 8.x`**
Copy link
Member

Choose a reason for hiding this comment

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

So one has to create this Dockerfile? Say that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

docs/build_custom_logstash_image.md Outdated

3. Build the gem by running the following command:

```bash
Copy link
Member

Choose a reason for hiding this comment

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

Align left, add a space after $.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I will standardize it to match the format used in other documentation files within the repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

docs/build_custom_logstash_image.md Outdated
$gem build logstash-output-opensearch.gemspec
```

It will generate for the gemfile `logstash-output-opensearch-2.0.2-x86_64-linux.gem`
Copy link
Member

Choose a reason for hiding this comment

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

In the local folder, not in pkg?

Copy link
Contributor Author

@FloMedja FloMedja Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The file is created at the root of the repo. I tried the command again.

@FloMedja FloMedja force-pushed the add-documentation-for-building-logstash-custom-image-with-opensearch-plugin branch 3 times, most recently from 63e6d29 to ceced46 Compare April 11, 2024 19:30
@FloMedja
Copy link
Contributor Author

@dblock @oeyh I integrate the comments and rebase. Thanks for taking another look.

oeyh
oeyh approved these changes Apr 11, 2024
View reviewed changes
Copy link
Collaborator

@oeyh oeyh left a comment

Choose a reason for hiding this comment

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

Thanks @FloMedja!

dblock
dblock requested changes Apr 12, 2024
View reviewed changes
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

The spelling of OpenSearch is a must have, everything else up to you. Will merge after.

DEVELOPER_GUIDE.md Outdated
@@ -9,6 +9,7 @@
- [Configuration for Logstash Output OpenSearch Plugin](#configuration-for-logstash-output-opensearch-plugin)
- [Submitting Changes](#submitting-changes)
- [Backports](#backports)
- [Dockerfile to build a custom Logstash image](docs/dockerfile_to_build_custom_logstash_image.md)
Copy link
Member

Choose a reason for hiding this comment

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

Let's just say "Building Custom Docker Images"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

DEVELOPER_GUIDE.md Outdated
@@ -215,3 +216,6 @@ original PR with an appropriate label `backport <backport-branch-name>` is merge
run successfully on the PR. For example, if a PR on main needs to be backported to `1.x` branch, add a label
`backport 1.x` to the PR and make sure the backport workflow runs on the PR along with other checks. Once this PR is
merged to main, the workflow will create a backport PR to the `1.x` branch.

## [Dockerfile to build a custom Logstash image](docs/dockerfile_to_build_custom_logstash_image.md)
Copy link
Member

Choose a reason for hiding this comment

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

Make this a section

# Building Custom Docker Images

See [this doc](docs/dockerfile_to_build_custom_logstash_image.md) for details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

docs/dockerfile_to_build_custom_logstash_image.md Outdated
@@ -0,0 +1,72 @@
# Dockerfile to build a custom Logstash image
Copy link
Member

Choose a reason for hiding this comment

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

Building Custom Docker Images

Filename can just be docker.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

docs/dockerfile_to_build_custom_logstash_image.md Outdated

## Logstash 8.x

Create this `Dockerfile` to build an image with `Opensearch 2.0.2` for **`Logstash 8.x`**
Copy link
Member

Choose a reason for hiding this comment

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

Unlike Elasticsearch, correct spelling is OpenSearch, and no need to quote since that's note code.

"Create a Dockerfile to build an image with OpenSearch 2.0.2 and Logstash 8.x."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes. Done.

docs/dockerfile_to_build_custom_logstash_image.md Outdated
```


3. Remove [this line that adds the json version spec](https://github.com/opensearch-project/logstash-output-opensearch/blob/2.0.2/logstash-output-opensearch.gemspec#L49)
Copy link
Member

Choose a reason for hiding this comment

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

Btw, why is this needed? Let's explain here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a little explanation.

docs/dockerfile_to_build_custom_logstash_image.md Outdated

### Dockerfile

Create this Dockerfile to build an image with `logstash version 7.x` and the previously generated `gemfile`:
Copy link
Member

Choose a reason for hiding this comment

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

Only quote code.

gemfile is Gemfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Add documentation for building Logstash custom image with opensearch …
e635c0c 
…plugin

Signed-off-by: Florian Medja <florian.medja@ubisoft.com>
@FloMedja FloMedja force-pushed the add-documentation-for-building-logstash-custom-image-with-opensearch-plugin branch from ceced46 to e635c0c Compare April 12, 2024 19:44
@FloMedja
Copy link
Contributor Author

@dblock I integrate the changes and generate a doctree. Thanks for the review :)

@dblock
Copy link
Member

dblock commented Apr 13, 2024

Thanks!

Would love more help on this project. There's a bunch of low hanging fruit documentation tasks like #234 or #141, but also plenty of meatier ones.

@dblock dblock merged commit 18aa049 into opensearch-project:main Apr 13, 2024
21 of 29 checks passed
@FloMedja FloMedja deleted the add-documentation-for-building-logstash-custom-image-with-opensearch-plugin branch April 13, 2024 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock approved these changes

@oeyh oeyh oeyh approved these changes

@asifsmohammed asifsmohammed Awaiting requested review from asifsmohammed

@dlvenable dlvenable Awaiting requested review from dlvenable dlvenable is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FloMedja @dblock @oeyh