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

Initial version of the SNMP integration plugin, incorporating snmp and snmptrap inputs #1

Merged
merged 8 commits into from
Feb 15, 2024

Conversation

edmocosta
Copy link
Contributor

@edmocosta edmocosta commented Jan 5, 2024

What does this PR do?

Created the SNMP Integration Plugin, incorporating the logstash-input-snmp and logstash-input-snmptrap plugins.

  • No code changes were done, only small fixes, adjustments and project structure moves.
  • A flaky test was fixed and the .ci adjusted to work with both plugins. It was mainly adapted from the original CI configurations.
  • Unit tests and integration tests were placed into two separate folder spec/integration and spec/unit. The specs were not modified.
  • Docs were adjusted to follow the integration plugins pattern:
    • index.asciidoc: summarizes the provided plugins
    • input-snmp.asciidoc: no changes from the original input plugin but the header :type: integration markups
    • input-snmptrap.asciidoc: no changes from the original input plugin but the header :type: integration markups

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Related issues

@edmocosta edmocosta marked this pull request as ready for review January 11, 2024 15:20
@edmocosta edmocosta changed the title [WIP] Initial version Initial version of the SNMP integration plugin, incorporating snmp and snmptrap inputs Jan 11, 2024
Copy link

@roaksoax roaksoax left a comment

Choose a reason for hiding this comment

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

I've gone through all of the plugin, given that the code is the same, and it's only organizational, I haven't gone through a thorough review. But LGTM!

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

@edmocosta, thank you for the opportunity to try to get these new integration docs working from the beginning. I've commented on some lines that I know will cause problems if unresolved, and added explanations. Please let me know if you'd like to discuss.

Let's check in again before you merge and publish.

Comment on lines 26 to 27
- {logstash-ref}/plugins-input-snmp.html[SNMP Input Plugin]
- {logstash-ref}/plugins-input-snmptrap.html[SNMP Trap Input Plugin]
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
- {logstash-ref}/plugins-input-snmp.html[SNMP Input Plugin]
- {logstash-ref}/plugins-input-snmptrap.html[SNMP Trap Input Plugin]
/ * {logstash-ref}/plugins-input-snmp.html[SNMP input plugin]
/ * {logstash-ref}/plugins-input-snmptrap.html[Snmptrap input plugin]

Copy link
Contributor

Choose a reason for hiding this comment

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

These links should be removed for the initial commit. Why?
The generated files were targeting don't exist yet. (We're merging the original source files as part of this PR.) We can't link to those generated files until they've been created, merged, published, and generated. Linking to non-existent files will break docs-ci, and if merged, the entire docs build.


=== SNMP input plugin

include::{include_path}/plugin_header-integration.asciidoc[]
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
include::{include_path}/plugin_header-integration.asciidoc[]
/ include::{include_path}/plugin_header-integration.asciidoc[]

Copy link
Contributor

Choose a reason for hiding this comment

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

This include statement pulls in this header file: https://github.com/elastic/logstash/blame/main/docs/include/plugin_header-integration.asciidoc.

The header file includes a reference to a file that does not exist yet: <<plugins-integrations-{integration},{integration} integration plugin>>.

We're merging the original source files as part of this PR. We can't link to those generated files until they've been created, merged, published, and generated. Linking to non-existent files will break docs-ci, and if merged, the entire docs build.

We need to comment out (or delete) this include statement until we have generated initial versions of the target files.


=== SNMP trap input plugin

include::{include_path}/plugin_header-integration.asciidoc[]
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
include::{include_path}/plugin_header-integration.asciidoc[]
/ include::{include_path}/plugin_header-integration.asciidoc[]

Copy link
Contributor

Choose a reason for hiding this comment

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

This include statement pulls in this header file: https://github.com/elastic/logstash/blame/main/docs/include/plugin_header-integration.asciidoc.

The header file includes a reference to a file that does not exist yet: <<plugins-integrations-{integration},{integration} integration plugin>>.

We're merging the original source files as part of this PR. We can't link to those generated files until they've been created, merged, published, and generated. Linking to non-existent files will break docs-ci, and if merged, the entire docs build.

We need to comment out (or delete) this include statement until we have generated initial versions of the target files.


=== SNMP Integration Plugin

include::{include_path}/plugin_header.asciidoc[]
Copy link
Contributor

Choose a reason for hiding this comment

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

The plugin_header.asciidoc file include can stay. Technically, the integration plugin itself (but not its component plugins) behaves like a traditional (input, output, codec, filter) plugin. Only the plugin_header-integration.asciidoc header file causes problems.

@karenzone
Copy link
Contributor

@edmocosta Thank you for the opportunity to work through adding an integration iteratively. We might still hit some bumps, but we'll learn the best practices and then I'll document them for next time.

@edmocosta edmocosta merged commit 19f89d0 into main Feb 15, 2024
3 checks passed
@edmocosta edmocosta deleted the initial-version branch February 15, 2024 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants