20:00:03 <nitzmahone> #startmeeting Ansible Windows Working Group
20:00:04 <zodbot> Meeting started Tue May 14 20:00:03 2019 UTC.
20:00:04 <zodbot> This meeting is logged and archived in a public location.
20:00:04 <zodbot> The chair is nitzmahone. Information about MeetBot at http://wiki.debian.org/MeetBot.
20:00:04 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
20:00:04 <zodbot> The meeting name has been set to 'ansible_windows_working_group'
20:00:11 <nitzmahone> #chair jborean93
20:00:11 <zodbot> Current chairs: jborean93 nitzmahone
20:00:12 <jhawkesworth> hey
20:00:26 <nitzmahone> #chair jhawkesworth
20:00:26 <zodbot> Current chairs: jborean93 jhawkesworth nitzmahone
20:00:30 <nitzmahone> howdy
20:00:31 <jborean93> hey
20:00:39 <Shachaf92> יקט
20:00:41 <Shachaf92> hey
20:01:44 * jborean93 gives 1 more minute before starting
20:04:25 <jborean93> alrighty let's get started
20:04:36 <jborean93> #topic https://github.com/ansible/community/issues/420#issuecomment-490255070
20:04:40 <jborean93> there are a few links in here
20:05:45 <jhawkesworth> shall we go top to bottom, start with https://github.com/ansible/ansible/pull/56196 ?
20:06:27 <jborean93> I needed to test it out and see how it handles become + no become but I've got no problems with adding that there
20:07:11 <jborean93> I actually have a WIP that tests out sharing the web code between modules that adds this https://github.com/ansible/ansible/pull/54759 but happy to introduce it in the modules first as 54759 isn't ready for prime time yet
20:07:26 <jhawkesworth> would it make sense to also have default network credentials ?
20:08:02 <nitzmahone> How does it handle cases where we don't have plaintext creds? (eg NTLM)
20:08:27 <jborean93> nitzmahone: probably does nothing (anon) but that's part of the become + no become testing
20:09:07 <Shachaf92> according to documentation before this change it was basiclly always false
20:09:30 <Shachaf92> so that part is clear
20:10:25 <nitzmahone> But yeah, I'm also +1 for adding this as a switch, just want to make sure it'll work in most cases
20:10:49 <nitzmahone> (+ document when it won't)
20:11:12 <jborean93> yep, the docs currently say ntlm will work but I would be surprised if it does
20:11:18 <jhawkesworth> yep anything to avoid 'it doesn't work' bug reports
20:11:22 <jborean93> So I'm find with the changes just needs some doc changes
20:11:36 <nitzmahone> yeah, I'm suspicious about NTLM-over-WinRM
20:11:59 <nitzmahone> I suspect they're saying you can auth *to* NTLM endpoints
20:12:06 <jhawkesworth> I have next to no experience of ntlm-over-winrm
20:12:13 <nitzmahone> It's ... fun
20:12:29 <Shachaf92> What docs should be changed?
20:12:38 <jborean93> https://github.com/ansible/ansible/pull/56196/files#diff-3f43d632d5cb72d688e114f52d2c6b87R154
20:12:46 <nitzmahone> just the docs for that option once we know for sure where it won't work
20:13:00 <jborean93> Expand to include become, test kerb, ntlm, credssp to see if they actually work
20:13:32 <jborean93> Maybe even an explicit entry to say basic, and/or others won't work unless become is used
20:13:47 <Shachaf92> i see
20:13:58 <Shachaf92> ill try to set up an environment to test it all
20:14:05 <Shachaf92> might take me some time
20:14:18 <jborean93> we've got some time fore 2.9 is feature freeze :)
20:14:35 <Shachaf92> k
20:15:09 <jborean93> ok now https://github.com/ansible/ansible/pull/56279
20:15:31 <Shachaf92> btw i have not seen any defaultnetworkcredentials stuff regarding System.Net.WebReques
20:15:47 <nitzmahone> nooo, 56279 is not gonna fly
20:16:13 <jborean93> yea, I can't remember what but there was an issue with using this before
20:16:23 <jborean93> sounds like nitzmahone remembers
20:16:26 <jhawkesworth> how come?
20:16:51 <nitzmahone> You're not guaranteed to have DNS, it can resolve to lots of different things, reverse DNS isn't always there, tons of problems with that
20:16:53 <jhawkesworth> does System.Net.Dns do odd stuff on machines with networking not fully set up or something
20:16:59 <nitzmahone> That too
20:17:23 <Shachaf92> the static method should not be impacted by network settings
20:17:29 <nitzmahone> IIRC we've actually reverted that same change in the past because it cause so many problems, but I could be thinking of another job
20:17:35 <Shachaf92> you seen that failures yourself?
20:17:42 <nitzmahone> It can return SocketException, it certainly can
20:18:15 <nitzmahone> Yes, I've personally been burned by that method several times
20:18:34 <Shachaf92> interesting
20:18:47 <jhawkesworth> hmm, wondering if `hostname` does the right thing?
20:18:56 <nitzmahone> On a domain-joined machine, it generally works fine, but for cloud stuff, early provisioning, there be dragons
20:19:30 <jhawkesworth> (as in .exe) .. but hate calling .exes if at all possible to avoid
20:19:30 <clcaldwell> hostname returns the netbios name, aswell
20:19:47 <clcaldwell> same 15 char limitation
20:19:49 <Shachaf92> actually i saw hostname command returns the full name
20:20:09 <Shachaf92> we can always get it from the registry
20:20:12 <jhawkesworth> bug report comes from aws usage as well
20:21:27 <clcaldwell> nvm, Shachaf92 is right, i was confusing it with a different one
20:22:10 <Shachaf92> HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\ComputerName
20:22:23 <nitzmahone> https://docs.microsoft.com/en-us/windows/desktop/api/sysinfoapi/nf-sysinfoapi-getcomputernameexw
20:23:18 <jborean93> `GetComputerNameExW` seems to be the best option. We can also add a specific fact for netbios name in case people want that as well
20:23:24 <nitzmahone> I think there's a local ADSI object as well
20:24:11 <jhawkesworth> makes sense to have full and netbios name facts
20:25:41 <Shachaf92> so i should change it to have both and use GetComputerNameExW?
20:25:48 * jborean93 what a cf with all these hostnames
20:26:15 <clcaldwell> If you are using 'getcomputernameexw', you still have to decide if you want ansible to pick the netbios or non-netbios computername as the 'hostname'
20:26:31 <jborean93> I think so, just reading through the types to see how we want to map it
20:26:36 <Shachaf92> it makes sense to have the hostname be the full one and just add netbios one
20:26:55 <nitzmahone> In most sane cases they'd be the same, but agree we should probably have both, and the default should be the full one, with netbios available under a diff key
20:27:03 <jhawkesworth> longest version becomes 'hostname'
20:27:09 <nitzmahone> yep
20:27:25 <Shachaf92> wait, why not get them from WMI?
20:27:28 <clcaldwell> I disagree, I would actually think the netbios one makes more sense for windows. See; https://github.com/ansible/ansible/issues/55283#issuecomment-486789375
20:27:37 <jborean93> we should also probably look at `ansible_fqdn`, I know there are a few reports around that not being correct
20:27:56 <nitzmahone> Yeah, IIRC it's constructed from the truncated value
20:28:46 * nitzmahone wonders how things like NTLM/CredSSP auth are affected by non-compliant hostnames
20:29:09 <clcaldwell> Although, it is definitely not sane for a sysadmin to allow his windows computer's name to differ from NetBIOS name
20:29:25 <jborean93> people like to be different :)
20:29:30 <Shachaf92> why not get the facts from Win32_ComputerSystem and caption is netbios and DNSHOSTNAME is full
20:30:47 <nitzmahone> That works too- I was pretty sure the full values were available on one of those, but couldn't find it offhand
20:30:59 <nitzmahone> Oh
20:31:09 <nitzmahone> The Caption-based values are bad
20:31:26 <Shachaf92> sec ill get the better property
20:31:27 <nitzmahone> IIRC they don't work across all the Windows versions we support
20:31:47 <jborean93> if it works I would prefer WMI over PInvoke
20:31:51 <nitzmahone> ditto
20:32:00 <Shachaf92> the computersystem props go way back from xp / 2003 onward
20:32:04 <jhawkesworth> for sure
20:32:10 <clcaldwell> + 1 for WMI over PInvoke
20:32:38 <Shachaf92> k, computersystem has a property of name which is NB
20:32:49 <Shachaf92> and another DNSHOSTNAME for full
20:32:49 <nitzmahone> Yeah, we just have to make sure they behave the same. The explicit ones are generally fine so long as they're documented back to 2008 et al, but the implicit stuff like "Caption" often changes between releases
20:33:11 <jborean93> ok, so sounds like try out `Win32_ComputerSystem` to get hostname source. Set `ansible_hostname` to use the DNS name, and add `ansible_netbios_name` to use the netbios name?
20:33:22 <jborean93> + make sure it is consistent across out supported OS versions
20:33:23 <nitzmahone> WFM
20:33:34 <jhawkesworth> yes, that's what I'd like
20:33:39 <nitzmahone> (and ensure  FQDN is built off DNS as well)
20:33:57 <Shachaf92> k
20:34:01 <nitzmahone> noice- thanks
20:34:36 <jhawkesworth> yeah thanks for tackling.  sorry it turned out to be 'a can of worms'
20:34:38 <clcaldwell> Are you sure you want to set the default hostname to the non-NetBIOS name? That is almost guaranteed to cause problems if someone is using long computernames and AD
20:35:06 <Shachaf92> well it is the correct way to use them...
20:35:08 <nitzmahone> It's going to break something either way
20:35:19 <clcaldwell> @Nitzamahone yeah, probably :)
20:35:24 <nitzmahone> I'd rather the Netbios be the explicitly-opted-in one
20:35:26 <jborean93> and considering netbios is the special one I would prefer that be it's own fact
20:35:30 <nitzmahone> yep
20:35:31 <jhawkesworth> all my hostnames are exactly 15 characters
20:35:54 <jborean93> ok moving on https://github.com/ansible/ansible/pull/55862
20:36:46 <jborean93> I would need to go through them to make sure everything is good
20:37:18 <jborean93> there's a few `| Out-Null` which is better to be `$null = ...` or `> $null`
20:37:59 <jhawkesworth> I don't really have anything new to add
20:38:10 <jborean93> I still see some `$log_path = $null` before you set the var which I don't see any reason why that needs to be done
20:38:11 <nitzmahone> (microperf, but yeah, if the PR is cleaning stuff up, it shouldn't add new things we know are bad)
20:39:34 <Shachaf92> It is a best practice to always assign your vars in scripts before assigning from functions, if you all have such strong feelings about the null ill remove it
20:39:49 <jborean93> where is this best practice, I've never heard of it?
20:39:49 <Shachaf92> :(
20:40:12 <jborean93> and I'm sure there are hundreds, if not thousands of locations where we are doing just that
20:40:16 <Shachaf92> Ever tried runnign a script sevreal times in a session and got a lot of problems?
20:40:39 <Shachaf92> dirty vars are a big issue in scripts
20:40:46 <Shachaf92> in all languages
20:40:48 <jborean93> sounds more like a scoping problem
20:40:51 <nitzmahone> These aren't run that way though, and it adds a lot of noise
20:41:01 <Shachaf92> k ill remove it
20:41:13 <nitzmahone> (PS not being the most terse language out there to begin with ;) )
20:41:39 <Shachaf92> im old school sometimes, hence my ; you mentioned in the past
20:42:07 <Shachaf92> what about the WMI comment?
20:42:16 <jborean93> which one?
20:42:33 <nitzmahone> conversion to Get-CimInstance + explicit method invoke
20:42:45 <jborean93> in win_domain_membership
20:42:49 <Shachaf92> dagwieers commented on the lib/ansible/modules/windows/win_domain_membership.ps1
20:42:52 <Shachaf92> yeah
20:43:34 <jborean93> eh, as long as it works. I would test it manually as we don't actually run any good CI tests on that module
20:43:42 <Shachaf92> i tested it
20:43:54 <Shachaf92> it didn't work at first :)
20:44:11 <Shachaf92> now it does
20:44:36 <Shachaf92> should i really seperate the two .reboot_required = $awf.RestartNeeded lines from this PR? maybe just add a changelog line about it?
20:44:37 <jborean93> I probably do agree with dagwieers around some stuff. The addition of return values (win_domain.ps1) shoudl be in separate PRs
20:44:40 <jhawkesworth> as long as it behaves the same on s2008 and later windows versions.
20:45:01 <jhawkesworth> I see dag's point about keeping the functional stuff in a separate PR. I know it feels like overkill but if any of the functional stuff winds up having to be reverted, you don't have to loose all the other non functional changes.
20:45:09 <clcaldwell> Not sure if it matters, but changing from WMI to CIM will break it on W2K8 unless you have explicitely installed PoSH 5
20:45:09 <jborean93> yep
20:45:21 <jborean93> CIM stuff was added in v3
20:45:26 <jborean93> which is a requirement for Ansible
20:45:44 <clcaldwell> :ThumbsUp:
20:45:58 <jhawkesworth> i forget is it 2008 or 2008r2 is lowest supported?
20:46:02 <nitzmahone> 2008
20:46:03 <jborean93> 2008
20:46:05 <nitzmahone> :(
20:46:11 * jborean93 although not for much longer :)
20:46:37 * jhawkesworth looking forward to 2020 : goodbye ps3 and python 2.x
20:46:39 <jborean93> anyway, will need to do another review on that one but unless there's any more questions lets move on
20:46:52 * nitzmahone prepares to dance on their graves
20:47:06 <nitzmahone> Although we can't ditch Python 2.7 until RHEL7 dies :(
20:47:09 <Shachaf92> not sure what to do then with the wmi
20:47:28 <jborean93> I'm fine with having it changed in that PR as long as you've tested it works
20:47:28 <Shachaf92> keep it?
20:47:34 <Shachaf92> k
20:48:00 <Shachaf92> can we negotiate on adding the reboot req change to the changelog instead of a new PR?
20:48:03 <jborean93> in regards to the last comment in that post around old PRs
20:48:09 <jhawkesworth> are we onto old prs?
20:48:16 <jborean93> we've mostly left them as they are
20:48:16 <jhawkesworth> ah yes we are...
20:48:43 <jhawkesworth> the bot takes care of unanswered user questions over time
20:48:50 <Shachaf92> ok
20:48:51 <jhawkesworth> do you have specific examples.
20:48:53 <jhawkesworth> ?
20:49:05 <Shachaf92> not atm
20:49:07 <jhawkesworth> things like feature requests just kinda hang around
20:49:13 <Shachaf92> maybe ill list some next meeting
20:49:33 <jhawkesworth> occassionaly I chase up old ones
20:49:58 <jhawkesworth> cool. would be good to clear out some ancient things.
20:51:27 <jborean93> sounds good
20:51:35 <jborean93> #topic https://github.com/ansible/community/issues/420#issuecomment-492061525
20:52:21 <jhawkesworth> this is https://github.com/ansible/ansible/pull/55109 ?
20:52:30 <jborean93> doesn't look like kvprasoon is here (unless they have a different IRC handle)
20:52:30 <jborean93> yep
20:53:04 <jhawkesworth> possibly anti social hour for kvprassoon
20:53:05 <jborean93> barring the doc error around `required: yes` not being in the right place the code looks ok to me
20:53:22 <Shachaf92> there is waht jhawkesworth wrote
20:53:23 <jhawkesworth> wasn't sure if it was itentional not checking the Grouping param
20:53:32 <Shachaf92> he is correct it looks like it is missing from that array
20:53:49 <jhawkesworth> just don't know if it makes sense to check it
20:54:00 <Shachaf92> i think so
20:54:05 <jborean93> ah I see what you mean
20:54:15 <Shachaf92> since two rules can differ only in group
20:54:46 <jborean93> well win_firewall_rule fails when it finds 2 rules that match on the name. There are a few things about that module that are problematic
20:54:49 <jhawkesworth> looks nice to have otherwise
20:55:06 <jborean93> well in the absence of the author t here's not much else to talk about. The 2 comments still stand
20:55:14 <jhawkesworth> agreed
20:55:25 <jborean93> #topic https://github.com/ansible/community/issues/420#issuecomment-492345381
20:55:48 <jborean93> Shachaf92: is the link wrong or am I missing something?
20:55:59 <Shachaf92> https://github.com/ansible/ansible/issues/56124
20:56:03 <jhawkesworth> I guess one to parameterise in the tests when its fixed.  Not really a windows issue, just some tests using the buckets
20:56:13 <jborean93> ah I see
20:56:26 <jhawkesworth> or did I misunderstand?
20:56:33 <Shachaf92> not familier with aws much but should we handle it now?
20:56:37 <jborean93> doesn't look like we actually need to change anything
20:56:41 <Shachaf92> it says it will stop working in september
20:56:47 <jborean93> it's only for newer buckets
20:57:11 <jborean93> as per mattclay's comment https://github.com/ansible/ansible/issues/56124#issuecomment-492365629
20:57:25 <Shachaf92> yeah just saw it
20:57:38 <jhawkesworth> I guess it will kill CI if/when they turn off the old style names, so will get fixed pronto then :-)
20:57:40 <Shachaf92> but if the new way works for the old, shouldn't we change it to work the same?
20:58:00 <Shachaf92> should i look into a PR for this?
20:58:20 <jborean93> if it works I don't see why not. I'm not saying we don't change it, just that there's no real reason to right now. E.g. there are bigger fish to fry
20:58:44 <Shachaf92> well, when one of my small fishes fry ill move to the big ones
20:58:52 <Shachaf92> so i don't mind working on that one too
20:58:58 <jborean93> fine with me
21:00:33 <jborean93> #topic open floor
21:00:55 <clcaldwell> Not really in the scope of this meeting, but if someone gets a chance, can you look at this PR and help me figure out why the CI tests are failing? It doesn't appear that any of the code I've added is what is causing it to fail, but of course it probably is. Thanks! https://github.com/ansible/ansible/pull/56061
21:01:00 <jhawkesworth> nothing new from me.  Hope 2.8.0 is coming in to land smoothly
21:01:44 <Shachaf92> i wondered why there is win_get_url and seperate win_uri if in teh dotnet classes behind they are derived from one another?
21:02:35 <jhawkesworth> clcaldwell - looks like its objecting to a trailing \ :
21:02:36 <jhawkesworth> win_pester-RiNDrV / /root/.ansible/test/tmp/win_pester-Kh6rVc-ÅÑŚÌβŁÈ/test/integration/targets/win_pester/tasks/test.yml:46 / [windows-2008] windows: win_pester : Run Pester test(s) located in a folder. Folder path end with '\' path={{test_win_pester_path}}\
21:03:02 <Shachaf92> the stacktrace lookslike an infinite recurse
21:03:21 <jborean93> we set the nested depth to 99
21:03:21 <nitzmahone> yeah, whatever's attempting to be serialized there is either way too big or has a cycle
21:03:23 <clcaldwell> Ah, interesting. Thanks!
21:03:46 <clcaldwell> I guess it is me :p
21:03:51 <jhawkesworth> Shachaf92: its historic I suppose.  win_get_url and win_uri echo the linux equivalents get_url and uri which have over time got a lot more features and also got a lot more like each other
21:04:00 <jborean93> Shachaf92: win_get_url is designed to download files, win_uri is designed to invoke HTTP methods on an endpoint
21:04:44 <Shachaf92> ok
21:05:30 <jhawkesworth> first few versions of win_get_url didn't even use the .net client classes - and didn't work too well either :-)
21:05:46 <jborean93> those were the days
21:05:59 <Shachaf92> maybe should consider joingin them using a utils module to cut the duplicate maintenance
21:06:10 <jborean93> that's what that PR I mentioned at the start did
21:06:23 <Shachaf92> which one?
21:06:26 <jborean93> https://github.com/ansible/ansible/pull/54759
21:06:35 <clcaldwell> For win_uri and wing_get_url , it might be helpful to modify the synopsis to point to the other module with a note on the difference
21:06:39 <Shachaf92> oh
21:06:45 <jborean93> although there are some things I want to change hence why it is a WIP
21:06:53 <Shachaf92> cool
21:07:34 <jborean93> clcaldwell: devel now has seealso https://docs.ansible.com/ansible/devel/modules/win_get_url_module.html
21:07:47 <jborean93> when 2.8 is officially released that will become latest
21:08:30 <Shachaf92> ansible_fqdn and domain should we change to use WMI also?
21:08:33 <jhawkesworth> clcaldwell see https://github.com/ansible/ansible/issues/39587 for jborean93's analysis of difference between get_url and win_get_url
21:08:34 <clcaldwell> :ThumbsUp:
21:08:35 <Shachaf92> in setup
21:08:52 <nitzmahone> Shachaf92: Yeah, probably so
21:09:03 <Shachaf92> k
21:09:20 <jborean93> IIRC there's an issue around that so once you have it fixed you can close that
21:09:24 <jhawkesworth> yeah lets be consistent.  win_hostname module too, although I think you said you had a separate PR for that
21:10:17 <jborean93> yep
21:10:29 <Shachaf92> yeah win_hsotname will be seperate
21:10:37 <jborean93> there will be an interesting question around how we can support changing each individual hostname as well
21:11:02 <nitzmahone> yeah, perhaps a separate netbios_name or something on win_hostname might be warranted
21:11:19 <nitzmahone> or just say "don't do that" ;)
21:11:27 <Shachaf92> +1 to that
21:11:35 <jborean93> anywho we are overtime, anything else pressing before we close the meeting?
21:11:40 <Shachaf92> we shouldn't always support wrong dids
21:11:41 <clcaldwell> For hostname, can we set some sort of imformational message if we notice the length will make the name longer than NetBIOS?
21:11:43 <Shachaf92> deeds?
21:11:44 <jhawkesworth> yeah one for the windows guide... :-)
21:11:59 <nitzmahone> clcaldwell: yeah, that'd be a good case for a runtime warning
21:14:16 <nitzmahone> if no other topics, closing in 5..
21:14:21 <nitzmahone> 4..
21:14:26 <nitzmahone> 3..
21:14:30 <nitzmahone> 2..
21:14:33 <nitzmahone> 1..
21:14:38 <nitzmahone> Thanks all- until next week!
21:14:45 <jhawkesworth> cheers all
21:14:46 <nitzmahone> #endmeeting