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

Adding feature to Execute the Inventory Command #134

Merged
merged 13 commits into from
Jan 19, 2024
Merged

Adding feature to Execute the Inventory Command #134

merged 13 commits into from
Jan 19, 2024

Conversation

chungeun-choi
Copy link
Contributor

@chungeun-choi chungeun-choi commented Jan 17, 2024

Hello go-ansible Contributors!

This is chung-eun, discussing the inventory-related feature (#132). Our project urgently needed this functionality, so I went ahead and developed it.

I'm eager to contribute these developments to the go-ansible project, which is why I've submitted this PR. While I've based my work on the existing contribution guidelines and source code, please don't hesitate to leave comments if there are areas for improvement! ☺️

Inventory

  • ansibleInventoryOptions: An object to create option flags for executing the ansible-inventory command.
  • ansibleInventoryCmd: The object that actually executes the ansible-inventory command.
  • inventory_test.go: A file containing test code to test the newly added features.

Special Notes

  1. As everyone knows, inventory does not involve actual server connection tasks, so ConnectionOptions and PrivilegeEscalationOptions have been excluded from the properties of ansibleInventoryCmd.
  2. ansibleInventoryOptions: This functionality has been developed based on the official ansible-inventory documentation

I'm not entirely sure which branch this PR should be based on, so I've made the request against the master branch for now. If it needs to be changed, please let me know.

@apenella
Copy link
Owner

apenella commented Jan 17, 2024

Thank you very much for your contribution @chungeun-choi I will review it during the next few days!

@chungeun-choi
Copy link
Contributor Author

@apenella Thank you for your reply! I'll be looking forward to your review!

Copy link
Owner

@apenella apenella left a comment

Choose a reason for hiding this comment

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

Hi @chungeun-choi !
Thank you very much for your contribution; it will be greatly appreciated by the community!

If you don't mind, before we proceed with merging, I'd like to request some additional work:

  • I noticed that ansible-inventory has the '--limit' option, and it might be beneficial to include it here.
  • Would you mind adding a description of that package in the readme file, specifically in the packages section?
  • Lastly, could I ask you to provide an example showcasing how to use the ansible-inventory feature?

Again thank you very much for your effort and contribution!

pkg/inventory/inventory_test.go Outdated Show resolved Hide resolved
pkg/inventory/inventory_test.go Outdated Show resolved Hide resolved
pkg/inventory/inventory_test.go Outdated Show resolved Hide resolved
pkg/inventory/inventory_test.go Outdated Show resolved Hide resolved
@chungeun-choi
Copy link
Contributor Author

@apenella Thanks for the feedback. It seems I missed a few things! I'll leave a comment again after I've made the necessary adjustments

@chungeun-choi
Copy link
Contributor Author

chungeun-choi commented Jan 19, 2024

@apenella HI!. I've completed all the tasks you requested and am leaving a comment. While checking, I noticed a few issues and made some modifications. Please refer to the content below!

Requested Items


  1. Addition of the --limit flag functionality and adding test code for it.
  2. Update the README.md to include information about this feature in the packages section.
  3. Add example code for how to use this feature in the examples section.
  4. Modify the review comments left in the test code

Modifications


I apologize for overlooking this feature during development, as it seemed to be a less frequently used one

  1. --vars is not for inputting additional variables, but is used with the --graph flag to show the variable values held by each Host.

    -> As a result, the vaulted encryption process has been removed (related functions deleted, and this item is removed from the test code).

  2. The placement of the 'pattern' was initially set at the end of the command, but this was causing problems in certain situations.

    -> The placement of the 'pattern' value has been moved to follow right after DefaultAnsibleInventoryBinary.

  3. Improved an issue where certain option flags were being set to “nil”.

@apenella
Copy link
Owner

@chungeun-choi you did a great job with that contribution!
Thanks so much!

@apenella apenella merged commit b3dcdcc into apenella:master Jan 19, 2024
1 check passed
@chungeun-choi
Copy link
Contributor Author

@apenella I'm happy that what I've developed can be helpful!

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.

2 participants