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