19:10:05 <jillr> #startmeeting ansible core irc public meeting
19:10:05 <zodbot> Meeting started Tue Jun  4 19:10:05 2019 UTC.
19:10:05 <zodbot> This meeting is logged and archived in a public location.
19:10:05 <zodbot> The chair is jillr. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:10:05 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:10:05 <zodbot> The meeting name has been set to 'ansible_core_irc_public_meeting'
19:10:18 <jillr> #topic https://github.com/ansible/ansible/pull/57100
19:10:38 <jillr> brtknr you're up :)
19:13:12 <jillr> is brtknr still here?
19:13:30 <brtknr> jillr: yes
19:13:47 <cyberpear> my comment is that the cherry-pick was done incorrectly
19:14:20 <brtknr> How so cyberpear
19:14:26 <cyberpear> should have been commit 18f22de67e8304aebf4e78e2dfa6fe7831e635a3 but looks like it may have only picked up part of that change
19:14:44 <cyberpear> (the giveaway is that the original PR number isn't in the Commit Title)
19:14:51 <shertel> This doesn't seem like a regression. Looks like the same problem exists on 2.6
19:15:54 <sdoran> The main thing to decide is whether or not this is consider critical for the purpose of backporting to 2.7.
19:16:08 <sdoran> *considered
19:16:14 <brtknr> cyberpear: this is my first backport so please help me understand
19:16:36 <cyberpear> brtknr: that's not the real issue here... as sdoran said, "is this critical?"
19:17:09 <shertel> brtknr: for reference though, here's a doc to help with making backport PRs https://docs.ansible.com/ansible/latest/community/development_process.html#backporting-merged-prs
19:17:22 <felixfontein> cyberpear: the original commit doesn't have the PR number in its commit message either
19:17:52 <brtknr> well, as I pointed out previously, using os_stack module with the latext 2.7.x ansible without supplying tags arg (the most common use case) leads to failure
19:18:01 <felixfontein> (it looks like it was cherry-picked from the wrong commit)
19:18:31 <brtknr> supplying tags arg produces warning when using openstacksdk version less than 0.28.0
19:19:05 <shertel> whether or not it's the most common use case seems unclear
19:20:37 <felixfontein> you could simply switch to 2.8.1 (assuming that contains the fix, and once it is released) instead of using 2.7.x (for which you would have to wait anyway)
19:20:38 <shertel> (I say that since it looks like it's been a bug at least since 2.6)
19:20:50 <jillr> it seems like it's not a regression, and a valid workaround is to update to a more recent openstacksdk right?
19:20:57 <brtknr> people do not usually supply tags arg to os_stack module in my understanding, in the current form, users get an error if they dont
19:21:06 <cyberpear> here's the original PR for reference: https://github.com/ansible/ansible/pull/56575
19:22:03 <brtknr> cyberpear: thats correct
19:22:50 <brtknr> jillr: umm no that wouldnt work either because updating to openstacksdk==0.28.0 only stops the warning
19:22:51 <jillr> openstacksdk latest is 0.29.0, which just released with train, so the vast majority of users will be on <0.28.0 though
19:23:16 <brtknr> there will still be an error if tags is not supplied
19:23:27 <jillr> so we dont have a good workaround then?
19:23:50 <brtknr> its a bug that I introduced myself, previously
19:24:41 <brtknr> because I did not consider a situation where openstacksdk is less than 0.28.0
19:24:55 <brtknr> Having tested it, it fails when tags is not provided
19:25:19 <brtknr> Sorry I keep repeating myself but I dont know how to explain it any better
19:25:44 <jillr> we need to set the parameters for why it should be critically backported, so it sounds to me like:
19:26:08 <jillr> the bug has existed for a while but a good workaround does not exist, and breaks users in a common use case (no tags used)
19:26:23 <jillr> should we vote based on that?
19:26:27 <felixfontein> has this bug been reported (in issues) and have people been complaining about it? from the fact that this bug seems to be around for some time, I would expect that there were some complaints if this is really critical
19:26:47 <brtknr> That is a good summary
19:28:15 <jillr> I'm not seeing any open issues, felixfontein
19:28:41 <jillr> let's take a vote then:
19:28:56 <jtanner> cloudnull weighed in, but maybe someone should go poke the other maintainers and get some +1 s-1s
19:29:40 <jtanner> mordred specifically
19:30:06 <alongchamps> I'm in agreement with what felixfontein said, how many issues have we had on it
19:30:32 <felixfontein> jillr: I couldn't find a closed one, either
19:31:11 <cyberpear> tells me that `os_stack` is infrequently used
19:31:21 <felixfontein> or everyone uses tags :)
19:31:33 <brtknr> I did not create an issue for it unfortunately
19:31:51 <brtknr> Described the problem in the PR, probably bad practice
19:32:03 <brtknr> (almost definitely bad practice)
19:32:10 <felixfontein> that's in general not a problem; but nobody else did either, and nobody commented in the PR that they had the same problem
19:32:26 <brtknr> Hmm okay...
19:32:39 <felixfontein> which indicates to me that either people never had this problem, or they didn't care enough to report it
19:33:08 <cyberpear> I assume this module is used only on OpenStack cluster provisioning, which only happens once in a cluster's lifecycle?
19:33:46 <brtknr> cyberpear: it works O.K during provisioning, its only during cluster "update"
19:34:04 <cyberpear> I see
19:34:04 <felixfontein> I don't mind bugs being fixed (I like that in fact :) ), but I'm not sure whether this is really important enough to be backported. if it is, I guess there would be a lot of other things which should be backported as well
19:34:06 <brtknr> so all subsequent calls are affected
19:34:46 <cyberpear> the fact that this works for provisioning but fails on updates after the fact makes it critical in my mind
19:35:06 <brtknr> okay, whatever you guys decide, I feel like I did my bit by raising the issue here and making the case
19:35:52 <jillr> brtknr we super appreciate the patch, but there doesnt appear to be enough impact to backport at this time
19:36:12 <brtknr> cyberpear: thats my point precisely, i wouldnt case so much if it was a provisioning failure
19:36:47 <jillr> you can try reaching out to the openstack community (perhaps in #openstack-ansible) and see if module maintainers would want to accept the patch
19:37:19 <brtknr> someone installing the latest 2.7.x may hit this issue and only realise when its too late
19:37:33 <brtknr> i tried reaching out to mnaser but cant get hold atm :(
19:37:35 <jillr> let's take a vote on https://github.com/ansible/ansible/pull/57100 y'all:
19:37:38 <cyberpear> unfortunately, the OpenStack community on Ansible is very non-responsive to non-existent...
19:38:07 <cyberpear> so vote: +1 "accept as critical"; -1 "reject as non-critical"
19:38:10 <jillr> I'm -1 due to lack of criticality
19:38:18 <cyberpear> +1 from me.  Seems critical to me.
19:38:39 <Goneri> +1
19:38:45 <brtknr> am I allowed to vote?
19:38:47 <cyberpear> (I don't think my vote formally counts, though.  Stating nonetheless.)
19:39:07 <jillr> brtknr: yep
19:39:21 <jtanner> shrews said he would comment after his current meeting is over
19:39:26 <brtknr> well okay +1 there :)
19:39:37 <shertel> I'm -1 but am happy to defer to the openstack maintainers. I agree with felixfontein that if this constitutes a critical bugfix then there are probably a lot of other higher-traffic bugfixes that should be backported as well.
19:39:38 <felixfontein> -0.5 from me since it is critical for the module, but not for ansible it seems. also, as I said, if this one is backported, there will probably also be more things which should be backported to 2.7
19:40:17 <alongchamps> -1 from me, including what felixfontein has mentioned already
19:41:08 <jillr> split -3.5 to +3, anyone else want to weigh in?
19:41:23 <jtanner> -1, openstack maintainers should be the decision makers
19:42:01 <sdoran> -1
19:42:17 <jillr> so we (core) will not accept 57100, but we'd encourage the openstack folks to take a look at it if it's critical
19:43:11 <jillr> #topic https://github.com/ansible/ansible/pull/57312
19:43:14 <jtanner> please note that in the PR
19:43:42 <jillr> this pr is already closed?
19:43:52 <cyberpear> this was discussed at a meeting last week.  bcoca is working a larger PR that includes this minor bug fix, but I don't see any progress on getting his merged and backported
19:44:20 <cyberpear> without this PR, ansible-2.8 is very annoying with a non-suppressable "default change warning"
19:44:35 <cyberpear> looks like bcoca closed it this morning... I'll get link to previous meeting minutes on the topic
19:44:50 <jillr> thx cyberpear
19:45:10 <cyberpear> https://meetbot.fedoraproject.org/ansible-meeting/2019-05-23/ansible_core_irc_public_meeting.2019-05-23-15.00.log.html#l-319
19:46:20 <cyberpear> (start of previous discussion: https://meetbot.fedoraproject.org/ansible-meeting/2019-05-23/ansible_core_irc_public_meeting.2019-05-23-15.00.log.html#l-167 )
19:46:34 <cyberpear> original discussion  where core decided this is a change that is desired/required: https://meetbot.fedoraproject.org/ansible-meeting/2019-04-16/ansible_core_public_irc_meeting.2019-04-16-19.01.log.html#1-49
19:47:30 <jillr> unfortunately b-coca is not here and I don't have any more status on his PR, unless someone else in attendence does
19:48:07 <cyberpear> the topic PR has the "sentinel" value as requested by abadger1999 last time it was discussed.
19:48:13 <cyberpear> here's bcoca's PR: https://github.com/ansible/ansible/pull/55474
19:48:26 <cyberpear> bcoca's seems orthogonal except that he also happens to make this fix
19:48:57 <cyberpear> I don't care if his is merged/backported, or mine is.  I just want the warning gone.  Mine was meant to be easy to review and backport.
19:48:58 <jtanner> coca has been distracted by something a bit more critical
19:49:10 <jtanner> not to devalue this specific issue
19:49:43 <cyberpear> can my change be merged to fix the immediate issue?
19:50:36 <cyberpear> then whatever coca comes up w/ in the end will override; whenever that happens to be?
19:50:42 <abadger1999> I'd be +1 to merging this fix.  It doesn't seem to interfere with what bcoca is doing and it would allow a fix to get in earlier.
19:50:57 <abadger1999> afaict, it does everything that we asked at the last meeting.
19:51:20 <jillr> tbh I havent seen either PR previously so I'm going to defer to the group without weighing in, vote?
19:51:39 <jillr> as it sounds like others are quite familiar with them
19:52:09 <abadger1999> (Also at the last meeting we hadd reservations about all of bcoca's fix being backportable... so in terms of backporting, it might go in as just pieces anyway)
19:52:35 <felixfontein> I'm also +1, it would be really good to fix this sooner than later since this warning is really annoying
19:52:54 <sdoran> It seems the bcoca's PR may not be backportable. And we did ask for small, targeted fix.
19:53:28 <sdoran> I'm trying to understand this more. I haven't used this setting at all.
19:53:51 <felixfontein> sdoran: then you probably don't have groups with dashes in them, otherwise you would have noticed this already ;)
19:54:22 <sdoran> Nope. I play too much by the rules. :)
19:54:28 <cyberpear> sdoran: the fix allows users to acknowledge and explicitly opt-out of the future default behavior for group nomes
19:54:48 <shertel> I'm also +1
19:55:25 <jillr> seems like we're in agreement then
19:55:38 <sivel> So according to bcoca, his PR is the larger fix, whereas #57312 is a smaller, non-complete fix
19:55:39 <felixfontein> I would guess bcoca is -1 on this
19:55:53 <sivel> as such, he is still working towards the larger complete fix
19:56:25 <shertel> I think it's a somewhat separate. If bcoca's cannot be backported, then I think this should be merged first and his rebased for the more complete improvement.
19:56:26 <sivel> I am deferring to bcoca on all matters related to this
19:56:42 <abadger1999> (Also note, we have to decide whether to slip the 2.8.1 release so that this will appear in 2.8.1 or let it appear naturally in 2.8.2)
19:56:50 <sivel> so this needs his eyes and discussion, which I don't think we are going to get today
19:57:02 <shertel> I don't think this should slip 2.8.1
19:57:05 <jillr> so leave it on the table for 2.8.2?
19:57:17 <sivel> So this topic just needs pushed until next meeting
19:57:20 <felixfontein> I would have liked this to be in 2.8.0, but it's definitely too late for that :)
19:57:55 <felixfontein> I also would like 2.8.1 to be released ASAP, so I'm really torn
19:57:57 <sivel> or preferably cyberpear should discuss this more in depth with bcoca, rather than trying to force something through in the meeting
19:58:18 <cyberpear> (I'd assumed it was going to be in 2.8.0, as well.  I'd originally opened a PR to fix it similar to this on Apr16 when this was first discussed, but closed it as a subset of bcoca's.)
19:58:20 <sivel> since there is obvious disagreement there, they should reconcile that disagreement
19:58:42 <abadger1999> This was discussed at last week's meeting and we seemed okay with this.
19:58:47 <cyberpear> I'm not sure what the disagreement is... I've asked for clarification and gotten no response except to say that my change is a subset of the other change
19:58:50 <abadger1999> bcoca voted against at last week's meeting.
19:59:05 <felixfontein> sivel: I think he tried several times, with the result that there wasn't a real discussion
19:59:15 <sivel> The only agreement at the last meeting is that `ignore` in the code is a bug
19:59:17 <abadger1999> I'd have to look to see if we had an actual vote though.
19:59:26 <sivel> that I remember
19:59:27 <abadger1999> No, there was more agreed than that.
19:59:43 <sivel> if a discussion lasts longer than 5 minutes, it's gone on too long
19:59:44 <felixfontein> I'm not sure whether the bug was that 'ignore' was present, or that 'ignore' was not documented
19:59:56 <cyberpear> (there's an orthogonal docs PR that was also closed as a subset of coca's PR that helps explain all the options here: https://github.com/ansible/ansible/pull/57318 ) -- being docs only, I suppose this could wait until the "complete" fix.
20:00:02 <sivel> I start wandering past 5 minutes of interaction
20:00:05 <abadger1999> Possibly also less.... (like felixfontein, I'm not sure we agreed on ignore)
20:00:26 <sivel> In the end, bcoca wrote the feature, he knows better than anyone what the intentions were
20:00:40 <sivel> and the inclusion of ignore in the code was not planned
20:00:56 <bcoca> sorry, almost missed this
20:00:56 <shertel> bcoca has documented ignore in his PR, so it seems intentional
20:01:00 <bcoca> 'ignore' is not hte issue
20:01:04 <cyberpear> I'm happy to copy/paste exactly bcoca's change to this file as a complete fix if that would make folks happier
20:01:10 <sivel> agreed, that is just the only thing I remember
20:01:17 <bcoca> the 'bug' is that deprecation notice is still happening, which was not fixed by original PR, but mine does
20:01:19 <cyberpear> I made this to be the minimum possible fix
20:01:33 <abadger1999> https://meetbot.fedoraproject.org/ansible-meeting/2019-05-23/ansible_core_irc_public_meeting.2019-05-23-15.00.log.html#l-319
20:01:38 <bcoca> deprecation happened even when user set the value, that was the main issue
20:01:38 <sivel> bcoca: in short, cyberpear doesn't understand why his PR isn't acceptable, as he and others believe they are slightly different issues
20:01:51 <abadger1999> origin is what we agreed on (and what cyberpear's new PR does)
20:01:56 <bcoca> cause there is more than just that, we also need to show the choices (currently config does not)
20:02:00 <cyberpear> correct.  I don't understand why my PR is not accessible.
20:02:02 <cyberpear> *acceptable
20:02:05 <bcoca> and the choices are not validated (which my pr does)
20:02:12 <bcoca> cyberpear: cause it is incomplete
20:02:16 <abadger1999> nitzmahone indicated that he wanted this (cyberpear's) PR since it was a small, targetted fix.
20:02:20 <bcoca> you are only fixing 1/3rd of issue
20:02:26 <felixfontein> one could also say that these are different bugs, and there's no need to fix all of them at once
20:02:46 * nitzmahone -> WWG, attention limited ;)
20:03:00 <bcoca> which one, he opened 5
20:03:04 <jillr> we're running over time, what next step can we take from here?
20:03:31 <sivel> As indicated, I think bcoca and cyberpear need to have a chat
20:03:32 <abadger1999> 16:19:31 <nitzmahone> Beyond that, I don't have strong feelings. If the behaviors are logically equivalent, merge something like cyberpear's and backport, then bcoca's can subsume for 2.9
20:03:34 <bcoca> yeah, i also need to focus on my matter at hand, i can review later
20:03:39 <shertel> bcoca's pr also checks origin. I like the bigger fix, but I think backporting supressing the warning would be nice.
20:04:08 <cyberpear> my PR in the topic checks origin, as requested last meeting
20:04:09 <shertel> it seems like validating options and fixing the warning can be separated
20:04:24 <shertel> cyberpear yes, just noting that both PRs do it now
20:05:14 <shertel> bcoca this one https://github.com/ansible/ansible/pull/57312
20:05:55 <abadger1999> Following on from nitzmahone's comment, and since people were asking mespecifically about backportability, I said this:
20:05:59 <abadger1999> 16:36:02 <abadger1999> yeah, so please, for backporting, get me a patch that just includes the never-origin change and I think that should definitely be acceptable.
20:05:59 <abadger1999> 16:36:04 <bcoca> no, it does not, otherwise you would not need change in your PR
20:05:59 <abadger1999> 16:36:20 <abadger1999> Other things can go into a separate backport PR if we want to discuss whether those are also allowable or not.
20:06:52 <jillr> it sounds to me like we would be in favor of accepting cyberpear's PR for 2.8.2 and backport, then bcoca will continue on the larger work for 2.9
20:06:59 <jillr> is that the right takeaway?
20:07:00 <abadger1999> So... I think that as long as the change doesn't make any problem worse or prevent bcoca from fixing the larger issue, then it should be accepted.
20:07:38 <abadger1999> Yeah, that's my take away from last week's meeting notes..
20:07:56 <jillr> anyone in disagreement?
20:08:25 <sdoran> No. That's what was asked and that's what was delivered in this PR. Small, targeted fix that checks origin.
20:08:42 <sdoran> Provided it does not interfere with bcoca's longer term fix.
20:09:11 <sdoran> I want to avoid Ansible seeming schizophrenic and the warning behavior changing between minor releases.
20:09:32 <jillr> rad.  so folks who have an opinion should review et al on 57312
20:09:38 <sdoran> I think we should accept this PR but cyberpear and bcoca need to talk more to ensure they are working in concert.
20:09:43 <jillr> #endmeeting