search

ansible-collections / microsoft.ad

Ansible collection for Active Directory management
GNU General Public License v3.0
39 stars 22 forks source link

Fix for long computer names #114

Closed aristotelos closed 5 months ago

aristotelos commented 5 months ago
SUMMARY

Fix issue with long computer hostnames.

Fixes #113

ISSUE TYPE
COMPONENT NAME

microsoft.ad.membership

softwarefactory-project-zuul[bot] commented 5 months ago

Build succeeded. https://ansible.softwarefactory-project.io/zuul/buildset/d8f96e5466824a8990863c2cd134345a

:heavy_check_mark: ansible-galaxy-importer SUCCESS in 4m 43s :heavy_check_mark: build-ansible-collection SUCCESS in 8m 58s

jborean93 commented 5 months ago

Thanks for the PR, from a basic look I don't see this being a problem but I'll have to look a bit deeper as I seem to recall GetHostName() may have been problematic in some scenarios. I could have been thinking of another API so I'll have a play around and get back to you!

jborean93 commented 5 months ago

I've opened https://github.com/aristotelos/microsoft.ad/pull/1 which contains the changelog fragment and changes to the existing manual tests to cover this new scenario. I've run them locally to ensure that this tests out your original issue as well as a few other edge cases to ensure we don't break it in the future.

I would probably recommend using $cs.DNSHostName and add DNSHostName to the -Property list on line 129 because:

This isn't something you need to change so I left it alone in my PR targeting your branch but it might be worth considering when/if I ever get to adding Unicode support to this module.

softwarefactory-project-zuul[bot] commented 5 months ago

Build succeeded. https://ansible.softwarefactory-project.io/zuul/buildset/9e89e907f09b42c49f2f564b478e6f99

:heavy_check_mark: ansible-galaxy-importer SUCCESS in 4m 49s :heavy_check_mark: build-ansible-collection SUCCESS in 8m 55s

jborean93 commented 5 months ago

Thanks for working on this!

jterpstra1 commented 4 months ago

@jborean93 can a new release be published with this merge? We're running into the same issue and a new release with this would be appreciated.

Sorry for hijacking this issue.

jborean93 commented 4 months ago

I'm hoping to merge in https://github.com/ansible-collections/microsoft.ad/pull/117 once it's ready before the next release. Hopefully it'll be out at the end of this week or next week.