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

migrate cluster_drs_recommendations module #70

Conversation

mikemorency
Copy link
Collaborator

SUMMARY

This migrates the cluster_drs_recommendations module from the community.vmware collection

The inputs have not changed.
The outputs have changed from the original module. This version reports a description of the applied recommendations as well as details about the vcenter task associated with the change

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

cluster_drs_recommendations

Copy link

codecov bot commented Sep 28, 2024

Codecov Report

Attention: Patch coverage is 41.07143% with 33 lines in your changes missing coverage. Please review.

Project coverage is 26.75%. Comparing base (013afa4) to head (a8e4dff).
Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
plugins/modules/cluster_drs_recommendations.py 41.07% 33 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #70      +/-   ##
==========================================
+ Coverage   26.09%   26.75%   +0.65%     
==========================================
  Files          19       21       +2     
  Lines        1667     1798     +131     
  Branches      331      347      +16     
==========================================
+ Hits          435      481      +46     
- Misses       1232     1317      +85     
Flag Coverage Δ
sanity 26.75% <41.07%> (+0.65%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anna-savina
Copy link
Collaborator

@mikemorency what do you think about next questions?

  1. Is there an alternative approach to testing this module?
    Currently, the test cluster is empty, and there are no recommendations, meaning the test doesn't actually perform anything.

  2. What should the result be if there are no recommendations?
    The community.vmware.vmware_cluster_drs_recommendations module records in the results that no changes occurred

    "recommendations_info": {
        "changed": false,
        "failed": false,
        "result": "No recommendations."
    }

while vmware.vmware.cluster_drs_recommendations shows that changes were made

    "recommendations_info": {
        "applied_recommendations": [],
        "changed": true,
        "failed": false
    }

@mikemorency
Copy link
Collaborator Author

@anna-savina

  1. Validating that recommendations are made and run requires a vcenter with multiple hosts running multiple VMs in an unbalanced way. I think setting up that testing scenario will take awhile. I think that might be too long for the PR tests we run, what do you think?
    I was going to mention this in our QE sync but maybe we could have a longer test that runs nightly or something. That could stand up an entire test vcenter and then fully test this and other modules. Or maybe we could create some test VM templates (esxi, rhel9, etc) so spinning up a new complete cluster is much faster?

  2. Your right, the module should not report that something has changed if theres no recommendations. Ill fix that

@anna-savina
Copy link
Collaborator

I think setting up that testing scenario will take awhile. I think that might be too long for the PR tests we run, what do you think?

yes, I agree with you. I didn't find right approach to test this module, thought maybe you have ideas.

I was going to mention this in our QE sync but maybe we could have a longer test that runs nightly or something.

I'm not sure about this. Let's discuss it during our QE sync meeting

Copy link
Collaborator

@anna-savina anna-savina left a comment

Choose a reason for hiding this comment

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

LGTM

@mikemorency mikemorency force-pushed the mm-feature/migrate-drs-recommendations branch from 50a222e to a8e4dff Compare October 1, 2024 13:20
@mikemorency mikemorency merged commit 0488cee into ansible-collections:main Oct 1, 2024
15 checks passed
@mikemorency mikemorency deleted the mm-feature/migrate-drs-recommendations branch October 1, 2024 13:28
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