-
-
Notifications
You must be signed in to change notification settings - Fork 228
feat(pool): add pool_membership resource
#2297
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
feat(pool): add pool_membership resource
#2297
Conversation
Signed-off-by: Stanislav Shamilov <[email protected]>
Signed-off-by: Stanislav Shamilov <[email protected]>
Signed-off-by: Stanislav Shamilov <[email protected]>
Signed-off-by: Stanislav Shamilov <[email protected]>
Signed-off-by: Stanislav Shamilov <[email protected]>
Signed-off-by: Stanislav Shamilov <[email protected]>
Signed-off-by: Stanislav Shamilov <[email protected]>
Summary of ChangesHello @shamilovstas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new proxmox_virtual_environment_pool_membership resource, which is a great addition for managing resource pool members declaratively. The implementation is well-structured, with clear separation of concerns between the resource logic, the data model, and comprehensive tests (unit and acceptance). The code is clean and follows the provider's patterns.
I've left a few minor suggestions for improvement, mainly concerning naming consistency in Go, a small bug in an error message, and a documentation clarification. Overall, this is a high-quality contribution.
Signed-off-by: Stanislav Shamilov <[email protected]>
pool_membership resourcepool_membership resource
|
Possible improvements: make |
bpg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this feeature @shamilovstas! ❤️
This is a solid improvement, and I think the next steps you highlighted make sense.
I have a few minor comments, also gemini flagged Id -> ID renaming, which i think also should be applied.
|
Signed-off-by: Stanislav Shamilov <[email protected]>
Signed-off-by: Stanislav Shamilov <[email protected]>
pool_membership resourcepool_membership resource
bpg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
|
@all-contributors please add @shamilovstas for code, testing |
|
I've put up a pull request to add @shamilovstas! 🎉 |
Contributor's Note
/docsfor any user-facing features or additions./fwprovider/testsfor any new or updated resources / data sources.make exampleto verify that the change works as expected.Description
This is a WIP pull request to add a
pool_membershipresource. This resource will allow to add VMs, CTs and storages to resource pools.Example
Currently,
proxmox_virtual_environment_vmandproxmox_virtual_environment_containerusepool_idattribute to add itself to a resource pool. Adding this resource will solve the following issues:pool_idin both resources. The level of support is different (proxmox_virtual_environment_vmsupports it better thanproxmox_virtual_environment_container)pool_idattribute ofproxmox_virtual_environment_containerforces replacement, which makes it quite inconvenient to use.pool_idwill require duplicated code inproxmox_virtual_environment_containerandproxmox_virtual_environment_vm, as the Proxmox Pool API treats CTs and VMS the same iwhen adding them to a pool.Next steps
If this pull request is merged, I suggest these following steps:
deprecatedthepool_idattribute inproxmox_virtual_environment_containerandproxmox_virtual_environment_vmmembersfield fromproxmox_virtual_environment_poolresource as it's currently read only. If the full list of members are required, IMO it's better to use the correspondingdatasourceCommunity Note
Closes #693