changing ip_address param to expect list

daarrn commented 3 months ago

Description

changing spec options, ip_address parameter is being changed from a 'str' type to a 'list' type

fixes issue #245

How Has This Been Tested?

validated the change using a local collection of lowlydba and was able to successfully create a multi subnet ag listener when the ip_address parameter was expecting a list

Types of changes

[x ] Bug fix (non-breaking change which fixes an issue) - Fixes #
[ ] New feature (non-breaking change which adds functionality)

Checklist:

[x ] I have read/followed the CONTRIBUTING document.
[x ] I have read/followed the PR Quick Start Guide
[x ] I have added tests to cover my changes or they are N/A.
[x ] I have added a changelog fragment describing the changes.
[ ] New module options/parameters include a version_added property.

daarrn commented 3 months ago

Hello, very first time attempting to contribute, please don't be shy to let me know if I missed performing something correctly in the contribution process. Thanks for your work!

DorBreger commented 3 months ago

I think you will need to update the type of the subnet_mask too.

daarrn commented 3 months ago

I think you will need to update the type of the subnet_mask too.

I think you are right that the change will be needed there as well. I do not have a use case to test that change and left it out of my request due to that

lowlydba commented 3 months ago

Hi @daarrn ! Thank you for the pull request. I'll try to take a look at it this weekend and provide feedback.

codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.69%. Comparing base (de0aac7) to head (ead4087). Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #246 +/- ## =========================================== - Coverage 100.00% 93.69% -6.31% =========================================== Files 33 64 +31 Lines 106 2205 +2099 =========================================== + Hits 106 2066 +1960 - Misses 0 139 +139 ``` | [Flag](https://app.codecov.io/gh/lowlydba/lowlydba.sqlserver/pull/246/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=John+McCall) | Coverage Δ | | |---|---|---| | [sanity](https://app.codecov.io/gh/lowlydba/lowlydba.sqlserver/pull/246/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=John+McCall) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=John+McCall#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

DorBreger commented 3 months ago

@daarrn I tested this on a couple of VMs on my laptop on two different subnets with different subnet masks, and I can confirm you would have to change the subnet mask parameter to a list too.

daarrn commented 2 months ago

The sanity checks are still failing due to the list changes not being updated in the ag_listener.py documentation file.

whoops, yup. Updated document file, thank you!

github-actions[bot] commented 2 months ago

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and the docs are now incorporated into main: https://lowlydba.github.io/lowlydba.sqlserver/branch/main

lowlydba commented 2 months ago

Looks great! Thanks again for the enhancement, this is a very nice upgrade.

lowlydba / lowlydba.sqlserver