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

Module listen ports facts extend output #4953

Conversation

PKehnel
Copy link
Contributor

@PKehnel PKehnel commented Jul 13, 2022

SUMMARY

Include more infos from netstat or ss to the output.

Mainly:

  • state
  • foreign address
  • process

As raised in #4762

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

listen_portcs_facts

ADDITIONAL INFORMATION

Changes:

  • adding the parameter: '-a', to netstat and ss to include all connections.
  • rework netstat to a more general solution.

Open topics:

  • first time trying to contribute to a public repo, even so the pull request is not 100% done I wanted to get some initial feedback.

  • is the approach I took okay or should we maybe add a parameter to listen_ports_facts to contain more information, while keeping the format of the current solution as standard.

  • docstring type, is there a convention for this repo, I didn't find that info

  • Should I rework ss to have the same setup then network

  • Testing:

    • What is the verbatim output I should add?
    • How to test SS, I would need to change docker version to for example a redhat 7 machine, but ansible-test integration listen_ports_facts --docker centos7 -v fails for me with Gather Factes -> unreachable
    • Testing Netstat: ansible-test integration listen_ports_facts --docker -v uses netstat is working successful
      with the warning: WARNING: Unable to determine context for the following test targets, they will be run on the target host: listen_ports_facts
  • how to run pre-commit hook for my changes?
    community/general/plugins/modules/system/listen_ports_facts.py

  • what is the verbatim output?

@gaetan-craft maybe you give me some initial feedback?

@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added WIP Work in progress feature This issue/PR relates to a feature request merge_commit This PR contains at least one merge commit. Please resolve! module module needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html new_contributor Help guide this first time contributor labels Jul 13, 2022
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels Jul 13, 2022
@PKehnel PKehnel force-pushed the module_listen_ports_facts_extend_output branch from 684fc60 to a21125c Compare July 14, 2022 11:41
@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added plugins plugin (any type) system and removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jul 14, 2022
@PKehnel
Copy link
Contributor Author

PKehnel commented Jul 14, 2022

@moonrail and @gaetan-craft could one of you give me an initial review and some feedback on my idea / thoughts.

  • is the approach I took okay or should we maybe add a parameter to listen_ports_facts to contain all information, while keeping the basic format of the current solution as standard. ( I prefer that solution actually)

  • What is the verbatim output I should add?

  • Also will try to add some more documentation

@PKehnel PKehnel marked this pull request as ready for review July 14, 2022 12:44
@PKehnel
Copy link
Contributor Author

PKehnel commented Jul 14, 2022

ready_for_review

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Just some generic comments :) (Also still needs a changelog fragment.)

plugins/modules/system/listen_ports_facts.py Outdated Show resolved Hide resolved
plugins/modules/system/listen_ports_facts.py Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Jul 17, 2022

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

You probably want to update the return value documentation (RETURN) to mention the new fields.

@ansibullbot
Copy link
Collaborator

@PKehnel this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jul 18, 2022
This was referenced Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request integration tests/integration module module new_contributor Help guide this first time contributor plugins plugin (any type) system tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants