20:02:24 <jborean93> #startmeeting Ansible Windows Working Group
20:02:24 <zodbot> Meeting started Tue Mar 19 20:02:24 2019 UTC.
20:02:24 <zodbot> This meeting is logged and archived in a public location.
20:02:24 <zodbot> The chair is jborean93. Information about MeetBot at http://wiki.debian.org/MeetBot.
20:02:24 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
20:02:24 <zodbot> The meeting name has been set to 'ansible_windows_working_group'
20:02:31 <jborean93> #chair jhawkesworth
20:02:31 <zodbot> Current chairs: jborean93 jhawkesworth
20:03:02 * nitzmahone alarm didn't go off to come in here, weird
20:03:32 <jborean93> #chair nitzmahone
20:03:32 <zodbot> Current chairs: jborean93 jhawkesworth nitzmahone
20:04:28 <jhawkesworth> hey
20:05:14 <jborean93> will give it just a few more minutes, have 2 items on the agenda I would like to bring up
20:05:20 <jhawkesworth> sure
20:05:23 <jborean93> I'm sure dag would be interested in them if he's here
20:07:22 <jborean93> alrighty let's get started
20:07:32 <jborean93> #info https://github.com/ansible/community/issues/420#issuecomment-469536173 null comparison pslint
20:07:40 <jhawkesworth> ok
20:07:49 <jhawkesworth> that rule has a dodgy sounding name already!
20:07:52 <jborean93> #topic https://github.com/ansible/community/issues/420#issuecomment-469536173 null comparison pslint
20:08:00 * jborean93 helps if I have the correct tag
20:08:33 <jborean93> basically doing something like `($var -eq $null)` can be bad and `$null` should always be on the left of a comparison operation
20:08:45 <jborean93> we had that rule but for some reason disabled it, I can't remember why
20:08:57 <nitzmahone> that old chestnut's been around since the early days of C
20:09:28 <nitzmahone> Yes, it can technically happen, but in PS it requires *two* failures
20:09:42 <nitzmahone> $var = $null instead of $var -eq $null
20:10:49 <nitzmahone> Even in C-ish languages most people ignore that rule and, you know, test their code
20:11:00 * nitzmahone isn't salty about that rule at all
20:11:19 <nitzmahone> I've always disliked the constant-on-left conditional style
20:11:38 <jhawkesworth> this guy also thinks its not great toohttps://github.com/PowerShell/PSScriptAnalyzer/issues/200
20:11:49 <jhawkesworth> oops try again without borked link
20:11:51 <jhawkesworth> https://github.com/PowerShell/PSScriptAnalyzer/issues/200
20:11:52 <jborean93> the trouble is if you do something like `$a = @(); if ($a -ne $null) { $true } else { $false }`
20:12:08 <jborean93> you would expect it to be `$true` but it is `$false
20:13:13 <jborean93> same if `$a = @($null)`
20:13:52 <nitzmahone> ah yeah, I forgot there's a little more to it because Powershell
20:14:21 * nitzmahone still isn't a fan of that rule
20:14:21 <jborean93> just reading issue 200 though
20:15:46 <jborean93> it still sounds like `$var -eq $null` is ok in a lot of situations but just not some. This leads into my next topic but sounds like we should still be defensive about it and just enforce $null on the left like the rule does
20:16:49 <jborean93> it seems wrong that you have to do that but we aren't able to change PowerShell
20:17:24 <jhawkesworth> yeah it doesn't make for easy readability
20:18:02 <jborean93> if we are checking for $null then in the majority of cases we want to check the value of var is null, not a item inside that collection. If we do actually want the latter, should be quite rare, we can ignore the rule just for that and make it explicitly clear why
20:21:33 <jhawkesworth> that makes sense to my done-too-much-java-programming mind.
20:21:52 <jborean93> nitzmahone: are you ok with that?
20:22:41 <nitzmahone> so *do* enforce the rule, or not?
20:23:06 * nitzmahone wasn't sure exactly what you were proposing
20:23:12 <jborean93> we enforce it
20:23:37 <nitzmahone> with the change made by #200, how many places do we have to use it?
20:23:40 <jborean93> it sounds like `$var -eq $null` is fine in most cases but not all, whereas `$null -eq $var` is typically what people want
20:23:43 <nitzmahone> * to make changes
20:24:09 <jborean93> pretty much the opposite of https://github.com/ansible/ansible/commit/307d8b5330b37b38008f3e821f5ff972b83ea4cf#diff-c2174afa441456cc772cf50514fce60c
20:24:22 <jborean93> we can slowly break that down by fixing the actual usages over time
20:25:16 <nitzmahone> not a fan, but if people believe it's going to prevent real issues, I can live with it
20:25:40 <jborean93> are you not a fan because it's unintuitive to do null on the left?
20:25:49 <nitzmahone> yeah, just really dislike that style
20:26:27 <jborean93> VSCode also highlights the lines which is purely cosmetic but does get the OCD out of me
20:26:35 <nitzmahone> :D
20:27:04 <jborean93> Ok, will raise a PR to re-enable it
20:27:08 <jhawkesworth> that is a big list of exclusions.  Do you know if there is any pattern to usages that cause it to fire?
20:27:46 * jhawkesworth looks for examples
20:28:15 <jborean93> it's mostly if the var is an array but that's hard for PSSA to analyse in the static way
20:29:04 <jborean93> Ok next topic is also from me
20:29:11 <jborean93> #topic https://github.com/ansible/community/issues/420#issuecomment-474200255 LiteralPath vs Path
20:29:21 <jhawkesworth> ok,  just wondered if its something that might lessen over time, as we move modules away from using ModuleUtils.Legacy
20:29:41 <jborean93> probably not, it's also an issue in areas outside of input module options
20:30:10 <jborean93> it will lessen if we have it activated as it will stop more code from being added that violates the rule
20:30:14 <jhawkesworth> ok, I'm happy with the rule being back on then
20:30:47 <jborean93> I've created a custom PSSA rule to detect the usages of `-Path` when `-LiteralPath` could also be used
20:30:48 <nitzmahone> jborean93 is spending a lot more time writing PS modules than I am these days anyway, so 🤷 ;)
20:31:10 <jborean93> Basically `-Path` accepts wildcard chars and one of those is the range operator `[]`
20:31:28 <jborean93> It means any path with `[]` in some form will fail when `-Path` is being used
20:31:36 <chopraaa> +1 for LP
20:31:50 <jborean93> I've updated a few modules recently so we can start testing it by default in CI but this rule will also pick it up in the sanity checks going forward
20:32:33 <chopraaa> I've never been able to run sanity checks on my dev box.
20:32:45 <chopraaa> Just starts and...fails
20:32:53 <jborean93> chopraaa: if you have docker installed then `ansible-test sanity --tests pslint --docker` should work
20:33:01 <jborean93> otherwise you need to have PSCore installed
20:33:09 <chopraaa> Oh neat
20:33:15 * jhawkesworth makes note
20:33:29 <jborean93> it's either `--tests` or `--test`, can't quite remember right now
20:33:33 <nitzmahone> yeah, `--docker` makes ansible-test a whole lot easier to work with
20:33:53 <nitzmahone> otherwise you have to install a *lot* of stuff (and the exact right versions) to make everything work
20:34:25 <nitzmahone> (at the expense of it being slower to actually start up since it has to bootstrap the container)
20:34:29 * jhawkesworth does most testing inside WSL... not tried to do docker inside it, but I suspect that won't fly
20:34:34 <chopraaa> I was looking at Molecule for running integration + sanity tests instead of the whole shebang we need to do right now
20:34:41 <nitzmahone> it actually does work
20:34:47 <jhawkesworth> wow
20:35:19 <mattclay> You can use `--tox` to make the requirements installation automatic, instead of using a container with `--docker`.
20:35:36 <jhawkesworth> tbh I tend not to run more that individual windows integration tests as my laptop is so slow train journey ends before tests do sometimes
20:35:40 <mattclay> Or provide your own virtualenv and use `--requirements`.
20:36:08 <chopraaa> There's not a lot on this in the ansible-test docs
20:36:42 <mattclay> And for `pslint` at least you can probably get away without any requirements installed (other than pwsh).
20:36:58 <jborean93> https://docs.ansible.com/ansible/devel/dev_guide/testing.html is a good starting point
20:37:11 <jborean93> breaks down into the various areas like `sanity`, `integration`, `windows-integration` and so on
20:37:21 <nitzmahone> I think it's gotten easier to install pwsh on most distros now; originally if you weren't running a recent Ubuntu, fat chance it would work.
20:37:36 <jhawkesworth> btw thanks for the write of debugging PS modules.  I would still be working on win_xml were it not for that!
20:37:39 <mattclay> So `ansible-test sanity --test pslint [file_to_test [...]]` should work if you have pwsh installed.
20:38:16 <chopraaa> Yeah I'll do that
20:38:21 <mattclay> Also, using ansible-test is much easier if you install and activate argcomplete.
20:38:28 <mattclay> Then you'll have tab completion.
20:38:35 <mattclay> At least if you're using bash for your shell.
20:39:19 <jborean93> getting back to the topic, what are people's thoughts about adding that custom rule to detect `-Path` vs `-LiteralPath`?
20:39:32 <jhawkesworth> I'm +1 for doing that
20:39:37 <chopraaa> +1
20:39:41 <nitzmahone> +1
20:39:44 <mattclay> +1
20:40:07 <jhawkesworth> jborean93 gets a clean sweep!
20:40:36 <jborean93> cool that makes it easy
20:40:52 <jborean93> #topic https://github.com/ansible/community/issues/420#issuecomment-474555702 module reviews
20:40:59 <jhawkesworth> -Path is probably wrong in most cases after all
20:41:22 <jborean93> I've been meaning to get to the win_nssm one before the freeze, probably don't really want to touch win_xml personally
20:41:58 <jhawkesworth> just that really.  I'm part way through testing win_nssm on my actual ansible configuration, after which I'll +1 it
20:42:14 <jhawkesworth> anyone else brave enough to open up win_xml?
20:42:21 * jborean93 runs far away
20:42:34 * nitzmahone hides
20:42:54 <jhawkesworth> if you'd rather it had a longer burn in I can s/changed in 2.8/changed in 2.9/
20:43:07 <jborean93> if the module author can review and he's happy I would just mmerge it
20:43:34 <jborean93> eh it's community and new, as long as it doesn't break things left right and center it probably doesn't need a long burn in time
20:43:40 <nitzmahone> ditto
20:43:46 <jhawkesworth> original module somewhat restricted, I wonder how much use it is getting
20:44:09 <jhawkesworth> ok well karstensrage was hoping to get to it this week
20:44:22 <jborean93> yep if he's happy then go ahead and merge
20:44:45 <jhawkesworth> thanks.  any body got anything else, I hope to have a bit of testing time over the next few days
20:44:52 <nitzmahone> nothing here
20:45:35 <chopraaa> There's win_format
20:45:46 <chopraaa> If anyone uses storage pools
20:46:09 <jhawkesworth> do you have the PR number to hand, may have a use for that
20:46:16 <nitzmahone> https://github.com/ansible/ansible/pull/53925
20:46:23 <jborean93> I think the community freeze has been pushed to next week, I probably won't have time to look at that this week but potentially next week
20:47:02 <nitzmahone> it has
20:47:39 <jhawkesworth> hmm, if I get other azure pre-requisites working I will try win_format PR
20:47:49 <chopraaa> Ive tested it for about a month but with no extreme usecases
20:47:50 <jhawkesworth> better than trying to script diskpart
20:47:53 <chopraaa> Like jons lol
20:48:04 <jborean93> cool well hopefully I get some time then. This week and next week is quite busy for myself
20:48:18 <chopraaa> Cheers
20:48:43 <chopraaa> Theres another question
20:48:56 <chopraaa> If theres interest in adding return values to win_partition
20:49:18 <chopraaa> And being able to support float values for size, like 5.2 GB
20:49:44 <chopraaa> Since there aren't any storage modules to go by im not sure whats okay and what isn't
20:50:15 <jborean93> I can't remember if we said to use the suffix size and then parse it from there
20:50:22 <jborean93> I'm pretty sure we did decide on that
20:50:26 <jhawkesworth> generally I'm a fan of return values if they tell you something you can then use in another module call
20:51:05 <jhawkesworth> personally I probably wouldn't have a need for float sizes
20:51:08 <chopraaa> jborean93: we did, but we're using integer values for size at the moment
20:51:08 <jborean93> I'm personally not a fan unless they tell you something you don't already know
20:51:40 <jborean93> ah sorry this is for return values not input option values
20:51:43 <chopraaa> If you create a partition and set drive_letter: auto, it creates a part using a free letter
20:52:09 <chopraaa> Which would be useful if returned (to format) i think
20:52:09 <jhawkesworth> that would be a good example of where you might then want the drive letter
20:52:31 <jborean93> yes in that case having a return value is good
20:52:39 <chopraaa> Didn't really have the foresight to implement this earlier
20:52:50 * chopraaa is apologetic
20:53:03 <jhawkesworth> its ok, iterating on ideas is good!
20:53:05 <jborean93> that's why we have preview interfaces :)
20:53:29 <chopraaa> Neat :-D
20:53:49 <jhawkesworth> well nothing else from me
20:54:12 <chopraaa> Same here
20:54:14 <jborean93> adding values is also quite easy, it's removing them that's a bit more problematic but in general I try to avoid return values unless it's something the module derived
20:54:29 <jborean93> For the return size, having the raw value is definitely a lot easier than trying to parse a string
20:55:03 <chopraaa> I'll go with bytes
20:55:20 <jborean93> sounds good to me
20:55:51 <jborean93> cool if there's nothing else will let you continue on your day
20:56:07 <jhawkesworth> yep, have a good one
20:56:10 <chopraaa> Good night folks
20:56:19 <jborean93> thanks all
20:56:22 <jborean93> #endmeeting