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

Add support for mDNS on network resource #292

Merged
merged 2 commits into from
Feb 24, 2023
Merged

Conversation

slok
Copy link
Contributor

@slok slok commented Nov 9, 2022

Hi!

First of all, I want to thank you for creating this awesome provider... In combination with Terraform cloud, my home network has gone to the next level 🚀. Many thanks :)

On Controller v7, mDNS was removed (#241) as a global setting and added into each of the networks independently.

Screenshot_20221109_150809

This PR adds the setting with a note that it's only supported on controllers >=v7.

Note: At first I added unit tests, but it started to get complex because this setting is not supported in all controller versions, also testing every single setting would make the tests hard to read and maintain, so I didn't add a test just for this small setting. If you think it's worth it, I can add the unit test.

Signed-off-by: Xabier Larrakoetxea <me@slok.dev>
@paultyng
Copy link
Owner

paultyng commented Nov 15, 2022

You can add a test but then make it skip if you aren't on the correct version, see a similar case here:

preCheckMinVersion(t, controllerVersionWPA3)

It doesn't need to cover every permutation but for regression purposes because controller versions and API change rapidly its nice to have it covered. I have this setup to run the tests on a schedule so it will continuously test against newer versions of the API so we can hopefully get out ahead of any future breaking change when possible.

Signed-off-by: Xabier Larrakoetxea <me@slok.dev>
@slok
Copy link
Contributor Author

slok commented Nov 22, 2022

@paultyng Test added! 🚀

@slok
Copy link
Contributor Author

slok commented Dec 21, 2022

Hi @paultyng!

The PR is ready, just a reminder in case it was forgotten, whenever you have time, please give it a review, many thanks!

@alexstophel
Copy link

👍 For this feature. It's essential for HomeKit to work properly and is the only part of my current network I can't provision with Terraform. Would love to see this merged and added.

@joshuaspence
Copy link
Collaborator

I merged a different PR which changes the function signature of getTestVLAN. But for some reason I am having trouble updating this PR so I will just merge this PR as is and fix it separately.

@joshuaspence joshuaspence merged commit 82cb711 into paultyng:main Feb 24, 2023
@slok
Copy link
Contributor Author

slok commented Feb 25, 2023

Thanks @joshuaspence!

@slok slok deleted the slok/mdns branch February 25, 2023 06:54
joshuaspence pushed a commit to chrishas35/terraform-provider-unifi that referenced this pull request Mar 1, 2023
* Add support for mDNS on network resource

Signed-off-by: Xabier Larrakoetxea <me@slok.dev>

* Add mDSN network tests

Signed-off-by: Xabier Larrakoetxea <me@slok.dev>

---------

Signed-off-by: Xabier Larrakoetxea <me@slok.dev>
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.

None yet

4 participants