15:05:31 <bcoca> #startmeeting ansible core public irc meeting
15:05:31 <zodbot> Meeting started Thu Jul 18 15:05:31 2019 UTC.
15:05:31 <zodbot> This meeting is logged and archived in a public location.
15:05:31 <zodbot> The chair is bcoca. Information about MeetBot at http://wiki.debian.org/MeetBot.
15:05:31 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
15:05:31 <zodbot> The meeting name has been set to 'ansible_core_public_irc_meeting'
15:05:50 <bcoca> #topic https://github.com/ansible/ansible/pull/49432
15:06:07 <zer0_her0> hello
15:06:16 <bcoca> @zer0_her0 .. im guessing you want review, its on my list but .. long list and lots of stuff distracting me
15:06:29 <bcoca> ^ if anyone else wants to pick up finalizing it
15:06:39 <zer0_her0> ok, i am pinging if we can manage it to the next release
15:07:17 <zer0_her0> this issue has been open since last December and I have incorporated all the requested changes
15:09:03 <felixfontein> I can confirm that bcoca's list is very long. I have some stuff on there for months, and I'm still waiting :D
15:09:27 <sdoran> I'll give it a final review.
15:09:41 <zer0_her0> i just reming that in 2.9 this is an opt in, default off feature
15:09:51 <bcoca> i feel like sisyphus sometimes, everytime i knock stuff off .. list still gets longer
15:10:28 <bcoca> zer0_her0: quick glance, seems ok, but its big enough change that requires careful rereview
15:10:29 <zer0_her0> bcoca, sorry, you just have been chosen :)
15:10:59 <bcoca> sdoran++ for stepping in
15:11:06 <felixfontein> you took the wrong pill
15:11:07 <bcoca> #topic https://github.com/ansible/ansible/pull/52649
15:11:20 <bcoca> i'm guesing same issue here, wanting review (btw, also on my list )
15:11:20 <sdoran> What did I step in? 😉
15:11:22 <zer0_her0> thanks, have a nice day
15:11:26 <bcoca> er .. up
15:11:30 <bcoca> s/in/up
15:11:36 <bcoca> though .. might be same thing ...
15:11:50 <sdoran> 😄
15:12:30 <rmahique> is just adding the option to remount a FS
15:12:45 <rmahique> useful for NFS
15:14:05 <bcoca> and bind mounts .. and any fs you change options
15:14:45 <bcoca> i thought we did -o remount already in some cases
15:15:00 <bcoca> doesnt this force 'always changed'?
15:15:29 <sdoran> We do that on BSD.
15:15:38 * bcoca double checks mount module, its been long time
15:15:56 <sdoran> Hmm, no scratch that.
15:16:15 <bcoca> think you are confusing 'relocatable fstab'
15:16:49 <bcoca> rmahique: can you give use case ?
15:17:05 <sdoran> Looks like we call `remount()` when a change is reported.
15:17:31 <bcoca> but this is to remount even if no change ?
15:18:05 <rmahique> bcoca, i thought to implemented it while i was at a customer and u can't do unmount then mount, remount makes sense with an NFS FS
15:18:17 <rmahique> yes
15:18:26 <bcoca> basically you are trying to 'fix an nfs mount' by remounting?
15:18:30 <rmahique> ie. if the NFS server is temporary down
15:18:37 <rmahique> yes
15:18:48 <sdoran> Yes, this always calls `remount()`. But it doesn't report a change.
15:18:51 <sdoran> Which it should.
15:19:02 <bcoca> sdoran: changed=true at end of if
15:19:12 <sdoran> 👍
15:19:16 <bcoca> it shoudl check if args changed also
15:19:45 <bcoca> ^ would be 'nice', but more worried about the description .. it just too plain, does not say 'its always changed' nor why you would want the optoin
15:19:59 <bcoca> code looks ok (brownie points if you actually return info about args also changing)
15:21:41 <sdoran> I don't know. Having an option that always results in a change seems wrong.
15:21:43 <rmahique> bcoca, do you want me to extend the description? "specifies that the device will be remounted. Useful for NFS FS, bindmounts, etc"
15:22:25 <bcoca> 'it always returns changed'
15:22:57 <bcoca> as for 'useful' .. i would point out the action/consequences more than mount types
15:23:09 <bcoca> 'for when you want to force a refresh on the mount itself'
15:23:14 <sdoran> Though I guess this could be used in a handler.
15:23:19 * akasurde waves
15:23:30 <rmahique> sdoran, it should be used when there is an issue with the FS, is not a normal situation
15:24:11 <rmahique> bcoca, i will add that now
15:24:19 <bcoca> k, think that closes the topic (we can keep going on details on ticket and #ansbile-devel)
15:24:25 <bcoca> #topic open floor
15:24:47 <bcoca> rmahique: also note' option added in 2.9'
15:25:11 <sdoran> Ok, that makes sense. Expanding the description should be sufficient.
15:26:55 <felixfontein> bcoca: how do you want to proceed with https://github.com/ansible/ansible/pull/58646 ? time between updating the PR and merging it should probably be pretty short, otherwise it ends up out of date pretty fast again...
15:27:19 <rmahique> bcoca, added
15:27:34 <bcoca> felixfontein: blocked by docs, they dont want to dsiplay string by default
15:27:54 <bcoca> i asked them to come to this meeting to litigate the issue
15:27:57 <felixfontein> ah, ok.
15:28:36 <felixfontein> was this discussed in the docs meeting on tuesday?
15:28:38 <bcoca> probably need to ping them again
15:28:42 <bcoca> no, was direct review
15:29:22 <felixfontein> otherwise, how about splitting the PR up into the validation part, and changing docs output part?
15:30:40 <bcoca> that requries decision here, since we made one conditional on the other
15:31:03 <bcoca> ^ that was my initial suggestion, but it lost to 'alwasy display string'
15:31:33 <felixfontein> ah, I missed that these are coupled.
15:32:00 <felixfontein> what do you think about https://github.com/ansible/ansible/pull/59060 ? (alias handling in validation)
15:32:03 <bcoca> by concious decision, it was specifically voted on that there were
15:32:18 <samccann> here but not familiar w/ the issue. acozine had a house emergency so can't make it (docs peeps)
15:32:47 <bcoca> lgtm, i would worry about performance but it seem to only impact error path, which we dont care about performance at that point
15:33:12 <bcoca> samccann:  mostly her concern wass that 'incorrect modules' would still display type: string
15:34:16 <felixfontein> bcoca: which part do you think could be slow? the joins?
15:34:42 <bcoca> again, not consideration for error path
15:34:53 <bcoca> at that point you dont care 'how fast modules exxecute'
15:36:20 <felixfontein> about incorrect modules: that are the ones where an entry for ignores.txt is added (or at least some of them, if we're only interested in the cases where some other type than str is specified in argspec, but no type is specified in option docs). going through that list and fixing them should be doable before 2.9, right?
15:36:40 <bcoca> all 4k .. possibly
15:36:51 <felixfontein> and if these ignores are gone, there are no more wrong things displayed anymore, I think
15:36:55 <bcoca> ~1400 files
15:39:12 <bcoca> felixfontein: to clarify my remarks above, thinking of using your code in runtime (moving to module_utils, sorry had this discussion withsomeone else and just added you to knowing the context in my head)
15:39:53 <bcoca> so we have  module: alias= , we report as 'actual option in error', would be nicer to report alias used with orig in (parens)
15:39:59 <bcoca> your code does 1/2 the work
15:40:07 <bcoca> .... i have too many chainsaws in flight
15:40:09 <felixfontein> ah
15:40:12 <felixfontein> yes :)
15:40:36 <bcoca> so my comment was about the PR that does not exist yet ...
15:40:43 <bcoca> that i reviewed in my head
15:41:05 * bcoca waits for men in white with nice vest to come and get him soon
15:43:35 <felixfontein> about the aliases problem, isn't that more like https://github.com/ansible/ansible/pull/53698 than https://github.com/ansible/ansible/pull/59060/files ?
15:43:56 <felixfontein> (the first PR checks whether multiple aliases of the same option have been used)
15:44:26 <bcoca> felixfontein: that is another, same issue of 'detecting alias'
15:44:36 <bcoca> so several issues to be fixed with that
15:45:23 <felixfontein> about the ~1500 ignore entries: if someone helps me get them merged quickly, I could work on that for some days, I think that should suffice to get it done
15:47:43 <bcoca> felixfontein++ for tackling mounds of work
15:48:01 <bcoca> sadly i dont think i have time to help right now
15:48:18 <bcoca> barely have time to rebase the pr to keep the list up2date
15:50:05 <felixfontein> do you mind if I rebase the PR?
15:50:27 <felixfontein> (assuming you didn't disable the checkbox :D )
15:51:52 <bcoca> go4it
15:52:45 <bcoca> just got under 50 open prs ...
15:53:26 <bcoca> https://github.com/ansible/ansible/pull/58461 <=  speaking of .. anyone want to review?
15:53:46 <bcoca> basically unobscure json errors when loading via dataloader
15:56:19 <felixfontein> mind if I move all ignore.txt changes into one commit? makes it easier to rebase
15:57:16 <Pilou> why 335/337/338 checks should be removed ?
15:57:24 <acozine> I'm back from meeting with the tree folks
15:58:02 <acozine> is dag around?
15:58:08 <felixfontein> Pilou: apparently not specifying type should be fine and default to 'str' (as before)
15:58:56 <acozine> my objections to defaulting to 'str' is that many undocumented parameters are not strings
15:59:01 <Pilou> wehn type is missing in documentation, type could be something else than str in argument_spec
15:59:32 <felixfontein> Pilou: when type is missing in docs and type is not str in argspec, it will still warn
15:59:34 <acozine> some of them are specified in code as non-string types, but also omitted from the arg spec
15:59:57 <acozine> yeah, if the argspec is correct, we should be able to fix it (though IIRC it's a lot of modules)
16:00:09 <bcoca> Pilou: 2 of them are now covered by the first
16:00:29 <bcoca> basically 'mismatch' as default would be string
16:00:50 <bcoca> the other were just cases of mismatch
16:00:55 <felixfontein> hmmm, modules with bad argspec are pretty bad. are there any? (I guess there are, probably the question should be: how many?)
16:01:04 <acozine> if it's a debate between "missing docs" and "misleading/wrong docs", I vote for missing
16:01:08 <bcoca> ~1800
16:01:32 <felixfontein> bcoca: I mean where argspec itself is wrong (i.e. type there is not equal to type used in module)
16:01:35 <Pilou> felixfontein: what is the error code of this warning ?
16:01:57 <bcoca> felixfontein: ah, no idea, that is just 'docs vs argspec mismatch'
16:02:13 <felixfontein> Pilou: 325
16:02:43 <felixfontein> docs vs. argspec mismatch is annoying, but fixable. wrong argspec is really worrying...
16:03:01 <bcoca> but that is deeper bug, which is out of scope for this pr
16:03:02 <felixfontein> fixable == relatively easily fixable
16:03:22 <acozine> I don't think the argspec entries are wrong, exactly, they just don't reflect all the settings in the code
16:03:23 <bcoca> and much harder to detect
16:03:43 <felixfontein> if type isn't specified in argspec, I think the value is converted to string currently?
16:03:45 <bcoca> acozine: argspec IS code
16:03:50 <felixfontein> so type: string would be "correct"
16:03:58 <bcoca> felixfontein: string is default, if not specified, yes
16:04:01 <bcoca> yes
16:04:03 <acozine> if a parameter is a dict in the code, and the argspec does not list a type, and the docs don't list a type either . . .
16:04:21 <bcoca> acozine: they would get a string, code would not be able to function
16:04:58 <bcoca> argspec default is string, you cannot provide a dict and expect it to work as dict
16:04:59 <acozine> I remember I had this same argument with dag back when he was submitting a lot of PRs adding `type: string` to the docs
16:04:59 <felixfontein> I don't know about windows modules, or other modules not using the Python AnsibleModule class, though... they could of course do random other things
16:05:26 <bcoca> acozine: docs vs argspec is one thing, if argspec does not match rest of code .. moduel would not work at all
16:05:31 <acozine> and I said "why can't we just assume the type is string"
16:05:47 <bcoca> felixfontein: yes, but no python modules in ansible/ansible NOT using argspec/ansiblemodule
16:05:58 <bcoca> acozine: but we DO assume it in the code
16:06:01 <bcoca> and enforce it
16:06:03 <acozine> hmm
16:06:21 <bcoca> when default was 'raw' , you are right, but default was changed to string, so it would not work
16:06:29 <acozine> well, then I must be misremembering the objection
16:06:30 <felixfontein> bcoca: ah good, I wasn't sure if some were still left... but then there are Windows modules, and they use something else, so no idea about them
16:06:44 <felixfontein> dag: you around?
16:06:52 <acozine> let me look at the old PRs and see if we had the argument in print at the time
16:06:53 <bcoca> they use similar, but im not sure about feature parity (i think its almost there)
16:07:15 <bcoca> acozine: probably docs/argspec mistmatch, not code/argspec
16:07:22 <bcoca> the latter would mean module is not functional
16:07:37 <felixfontein> bcoca: are there also sanity checks to verify that their code's argspec equals the docs?
16:08:01 <acozine> felixfontein: yes, there's a sanity check for that, with hundreds of exceptions
16:08:10 <acozine> oh, you mean the Windows modules
16:08:11 <acozine> never mind
16:08:46 <bcoca> felixfontein: my PR updates that specific check
16:08:58 <bcoca> there were 3 checks that did so partially before, i consolidate in my PR
16:09:11 <felixfontein> bcoca: but only for Python modules, right?
16:09:13 <bcoca> yep
16:09:17 <felixfontein> I mean for non-Python modules
16:09:22 <bcoca> idk
16:09:22 <felixfontein> (i.e. windows modules)
16:09:27 <felixfontein> me neither
16:09:30 <bcoca> dont know that we validate them at all
16:09:36 <bcoca> @jborean93 @nitzmahone ?
16:09:54 <bcoca> ^ do we validate-modules on windows? if so does it check for docs/argspec matching?
16:10:25 <bcoca> ^ not waiting, both are bussier than me and 'timezones'
16:10:32 <bcoca> i'll follow up on that
16:11:09 <felixfontein> thanks!
16:11:33 <bcoca> and sivel (author of validate) is on PTO
16:11:50 <felixfontein> if there's no validation (yet), Windows docs might be screwed up when default docs value for type is printed as "string" instead of "-
16:12:06 <bcoca> i would argue, they are probably already screwed up
16:13:21 <felixfontein> bcoca: https://github.com/ansible/ansible/pull/53698 <-- I didn't use warn() because no_log isn't set up at that point, and warn() calls log()
16:13:29 <acozine> possibly, but they currently advertise their screwed-up bits
16:13:55 <acozine> I'm concerned about "papering over" mismatches and mistakes
16:14:28 <bcoca> felixfontein: updated ticket
16:14:33 <Pilou> +1 for #58646, it's way better than was was done: only one test and str by default
16:14:38 <felixfontein> so we need Windows module validation before we can continue with #58646, if we don't want to split it up?
16:15:18 <felixfontein> bcoca: thanks! mind if I merge #53698?
16:15:47 <bcoca> i think we need to revisit the decision of 'display st by default' and postpone/split the functions, fix test now and deal with display later on once windows/current bad modules are sorted
16:15:59 <bcoca> felixfontein: go4it
16:16:35 <bcoca> ^ but it snot a decision i can revoke alone, needs to be a vote, since a vote was made to implement that way
16:16:52 <acozine> +1 for a phased approach on the string-by-default implementation
16:17:03 <bcoca> so .. those in favor of spliting 'dispaly str by default' from the PR?
16:17:06 <felixfontein> sounds good to me! so once display is gone and this is merged, I'll try to get rid of many of the ignore entries. someone else has to do Windows module validation though :)
16:17:07 <bcoca> +1
16:17:11 <felixfontein> +1
16:17:18 <bcoca> 3/0 .. needs more!
16:17:42 <Pilou> +1
16:18:11 <bcoca> hmm, might need to repost on tuesday to get quorum
16:18:21 <bcoca> and we are over time
16:18:51 <bcoca> giving 2 more mins in case people read and want to vote
16:19:27 <felixfontein> sounds good. @bcoca, about rebasing: do you mind if I split ignore.txt updates from other commits?
16:19:46 <bcoca> felixfontein: feel free to create even new PR , cherry pick what you need and close mine
16:20:26 <felixfontein> I don't mind keeping your PR, except if you don't want that :)
16:20:37 <bcoca> with that, going to close meeting, revisit on tuesday meeting to try to get quorum/votes
16:20:40 <bcoca> #endmeeting