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

vmware_first_class_disk_info - Add a module to gather infos about first class disks #1996

Conversation

Nina2244
Copy link
Contributor

@Nina2244 Nina2244 commented Feb 8, 2024

SUMMARY

Add a module to gather infos about first class disks
Fixes #1988

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

vmware.py-> find_first_class_disks(self, datastore_obj)
vmware_first_class_disk_info.py

ADDITIONAL INFORMATION

Copy link

Build failed.
https://ansible.softwarefactory-project.io/zuul/buildset/4cf3b870891b445793bd03ea8fce3dfc

⚠️ ansible-tox-linters CANCELED
build-ansible-collection FAILURE in 6m 26s
⚠️ ansible-test-cloud-integration-vcenter7_only-stable216 SKIPPED Skipped due to failed job build-ansible-collection
⚠️ ansible-test-cloud-integration-vcenter7_2esxi-stable216 SKIPPED Skipped due to failed job build-ansible-collection
⚠️ ansible-test-cloud-integration-vcenter7_1esxi-stable216_1_of_2 SKIPPED Skipped due to failed job build-ansible-collection
⚠️ ansible-test-cloud-integration-vcenter7_1esxi-stable216_2_of_2 SKIPPED Skipped due to failed job build-ansible-collection
⚠️ ansible-galaxy-importer SKIPPED Skipped due to failed job build-ansible-collection

@Nina2244
Copy link
Contributor Author

Nina2244 commented Feb 8, 2024

@ihumster do you know if I need to write tests?

@ihumster
Copy link
Collaborator

ihumster commented Feb 8, 2024

If it possible, of course.

missed the BaseConfigInfo Üroperties
@mariolenz
Copy link
Collaborator

recheck

@mariolenz
Copy link
Collaborator

recheck

@mariolenz
Copy link
Collaborator

recheck

Copy link
Collaborator

@mariolenz mariolenz left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests!

But I think we need validate_certs: false in some places to make the CI happy because we don't have official / trusted certs there.

BTW I think you should remove the changelog fragment. It's not needed, new modules should show up in the changelog automatically ;-)

But you should add the module to community.vmware/meta/runtime.yml.

I didn't find the time to test the module, though :-/

@Nina2244 Nina2244 requested a review from mariolenz February 20, 2024 07:09
@mariolenz
Copy link
Collaborator

recheck

Copy link
Collaborator

@mariolenz mariolenz left a comment

Choose a reason for hiding this comment

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

@Nina2244 I'm trying to understand why the CI fails and it looks like result_create_vcenter isn't registered anywhere. It might be a good thing to have this to troubleshoot the CI, but then 1) you should register the result when creating the FCD and 2) move it upwards so this this would be logged directly after the task that (tries to) create the FCD.

Alternatively, just remove this. We could add it later when we think this necessary.

@Nina2244 Nina2244 requested a review from mariolenz February 20, 2024 18:10
Copy link
Collaborator

@mariolenz mariolenz left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @Nina2244! I've tested this and the CI is happy now, too.

Special thanks for the integration tests. They are really helpful to avoid problems when there are changes to the module in the future!

@mariolenz mariolenz added feature This issue/PR relates to a feature request integration tests/integration new_module New module mergeit labels Feb 21, 2024
Copy link

Build succeeded (gate pipeline).
https://ansible.softwarefactory-project.io/zuul/buildset/665f17439e0f45c5a0ee1f379996c928

✔️ ansible-tox-linters SUCCESS in 10m 43s
✔️ build-ansible-collection SUCCESS in 8m 16s
✔️ ansible-galaxy-importer SUCCESS in 3m 43s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 1ee1822 into ansible-collections:main Feb 21, 2024
13 checks passed
@Nina2244
Copy link
Contributor Author

@mariolenz
Copy link
Collaborator

I'm not sure if I understand the question... as far as I understand, FCDs (or whatever the officially used name is) are independent from VSAN.

I've tested the module with FCDs on a VSAN datastore and it worked. I don't know if this helps you or answers your question.

@Nina2244
Copy link
Contributor Author

Its becaus @gnarc like to have some more funkitons and for this we/I need the VSAN Mgmt API: CnsQueryVolume Funktion.

@mariolenz
Copy link
Collaborator

You mean this comment and the ones after?

I'm not really sure. I've just cloned pyVmomi and ran grep -ri cns . but didn't find anything. This API might be available when using the vSAN Management SDK for Python. But it's not part of pyVmomi: vmware/pyvmomi#909

@Nina2244
Copy link
Contributor Author

Yes.

Thanks for the informations :)

@mariolenz
Copy link
Collaborator

I would really like to have this as a part of pyVmomi, I don't know why VMware doesn't simply include it 😩

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 mergeit new_module New module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add module to obtain fcd info (vmware_first_class_disk_info)
3 participants