| opendevreview | Michael Still proposed openstack/ansible-collections-openstack master: Use importlib.metadata to read openstacksdk version. https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992526 | 01:03 |
|---|---|---|
| opendevreview | Michael Still proposed openstack/ansible-collections-openstack master: Use importlib.metadata to read openstacksdk version. https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992526 | 02:21 |
| mnasiadka | noonedeadpunk: I think we need to prioritise https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992526 and a release… | 05:47 |
| noonedeadpunk | I have stumbled upon magnum CI failures last time | 07:00 |
| noonedeadpunk | due to heat removal | 07:00 |
| noonedeadpunk | which was https://review.opendev.org/c/openstack/ansible-collections-openstack/+/990454 | 07:00 |
| mnasiadka | Yeah, I can fix that later | 07:02 |
| mnasiadka | But since openstacksdk dropped version - everything is failing with newest openstacksdk | 07:02 |
| noonedeadpunk | I was thinking it was blocking rest of tests, but seems I was wrong | 07:02 |
| * noonedeadpunk revieweing | 07:18 | |
| opendevreview | Thomas Bechtold proposed openstack/ansible-collections-openstack master: Read openstacksdk version from package metadata https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992589 | 07:26 |
| noonedeadpunk | I will check on alternative approaches and will try to get a separate method to module_utils | 08:00 |
| noonedeadpunk | as this sounds like a great opprotunity to make this right and reduce amount of work to be done in case of future changes | 08:01 |
| mikal | noonedeadpunk: I don't see how the two are incompatible? Why would you not take the fix which unbreaks things, and then just land the style refactor when its ready? Its strange to me to hold up a build breakage fix on unrelated style concerns. | 08:15 |
| noonedeadpunk | why to do the same work twice? | 08:15 |
| noonedeadpunk | and also I am too used that "later" means actually "never" | 08:16 |
| noonedeadpunk | so it's better to do cleanly from the beginning | 08:16 |
| noonedeadpunk | while it's relevant | 08:16 |
| noonedeadpunk | but also - using older sdk version in kolla also works, right? so why not to patch it then to unbreak things? | 08:17 |
| mikal | Its not the same work. Its a refactor that you thought of because you saw code in the same area. I don't think its completely reasonable to try to "force" me to do unrelated work because as a user of your project it stopped working. | 08:18 |
| mikal | It might be a completely fine refactor, but you were happy with the implementation yesterday. | 08:19 |
| noonedeadpunk | I have not seen it yesterday, and I see it now it can be done better :) | 08:20 |
| mikal | https://review.opendev.org/c/openstack/kolla/+/992483 is already proposed to pin to before openstacksdk changed, but you know that's not a long term fix and I want to emphasise that _all_ users of the collection are broken, not just Kolla. | 08:20 |
| mikal | noonedeadpunk: sure, so propose a patch on top of the fix. | 08:20 |
| noonedeadpunk | we;re arguing longer then I can fix it tbh, so just give me like 20 mins to propose a patrch | 08:20 |
| mikal | noonedeadpunk: you do you man. | 08:20 |
| mnasiadka | noonedeadpunk: 20 minutes passed :) | 08:40 |
| * noonedeadpunk trying to figure out why we don't do any unit tests | 08:41 | |
| mnasiadka | Let’s not try to reimplement them right now | 08:42 |
| noonedeadpunk | I have some unit tests I wanna run, not bring back testing itself, but seeing I don't get any regressions is kinda nice | 08:44 |
| noonedeadpunk | okay, I'm giving up on unit tests.... | 09:19 |
| mnasiadka | noonedeadpunk: I think we could accept the work that mikal has made and put his cycles into it - maybe let’s merge the working version and follow up with something better later on? | 09:20 |
| opendevreview | Dmitriy Rabotyagov proposed openstack/ansible-collections-openstack master: Replace usage of removed openstack.version https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992598 | 09:22 |
| opendevreview | Dmitriy Rabotyagov proposed openstack/ansible-collections-openstack master: Replace usage of removed openstack.version https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992598 | 09:28 |
| noonedeadpunk | mnasiadka: won't you say it looks cleaner this way? ^ | 09:31 |
| noonedeadpunk | I am not sure really what was it all arguing about, as it's nothing unreasonable or too complicated... | 09:32 |
| mnasiadka | noonedeadpunk: well, I think it would make sense to add some attribution to mikal for his prior work | 09:35 |
| noonedeadpunk | oh, yes, sure | 09:36 |
| noonedeadpunk | that is really great point | 09:36 |
| noonedeadpunk | and it's also failing in CI now, so ... :D | 09:36 |
| mikal | That's why it wasn't a 20 minute problem. CI takes hours. I've already spent three hours or so on this problem today, which I think is a pretty big donation to a project I have never previously interacted with. Being asked to make unrelated style changes to me feels like I tried to donate $10 and was told I needed to add another $5 for it to be | 09:42 |
| mikal | accepted. In my mind a fix code review is completely different from a new feature in how much unrelated style work should be requested. But then again, I am just a guy and its clear you don't agree with me. | 09:42 |
| opendevreview | Dmitriy Rabotyagov proposed openstack/ansible-collections-openstack master: Replace usage of removed openstack.version https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992598 | 09:50 |
| opendevreview | Dmitriy Rabotyagov proposed openstack/ansible-collections-openstack master: Replace usage of removed openstack.version https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992598 | 09:50 |
| noonedeadpunk | this repo really needs some love... ugh | 09:51 |
| noonedeadpunk | mikal: well, introduction of new method was - I just started looking at unit tests, and that was not working nicely and really requires some time investment. I am not sure how these style changes are considered as unrelated, because they were specifically the changes you proposed. | 09:53 |
| noonedeadpunk | for me code review process is the same whether it's a new feature, bug fix or new contribution - all of that should be up to some level of readbility and general style. | 09:54 |
| noonedeadpunk | while I kinda see where you coming from, I indeed just don't think that smth too unreasonable has been asked... | 09:55 |
| noonedeadpunk | Hm, but also I kinda wonder about packaging missing. You wrote that it's exactly the reason why you are having try/except... | 09:57 |
| mikal | The refactor request was driven by the fact that the lookup happens in three places. That was true before the fix, and therefore is an unrelated refactor. I'm not opposed to that refactor happening, it seems like a good idea to me. My point is more that as a donor of my time, I do not see that as a particularly high priority to _me_ _right_ _now_. | 09:57 |
| mikal | The lesson here for me seems to be "just wait for someone else to fix it next time". | 09:57 |
| mikal | noonedeadpunk: the sanity-import test runs in an isolated venv that doesn't install the collection's requirements.txt. So adding packaging to requirements.txt is not sufficient. As mentioned on the review, this is visible in the several CI iterations I went through today. | 10:00 |
| noonedeadpunk | right, but not it's installing requirements.txt with https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992598/4/tox.ini | 10:00 |
| noonedeadpunk | and https://zuul.opendev.org/t/openstack/build/e0af3849bdb44bc0beb2a54faa34bdb0/log/job-output.txt#914 says it's installed | 10:01 |
| noonedeadpunk | aha, I see what you mean | 10:02 |
| noonedeadpunk | omg /o\ | 10:02 |
| noonedeadpunk | actually, I think this is a valid failure.... | 10:29 |
| opendevreview | Dmitriy Rabotyagov proposed openstack/ansible-collections-openstack master: Replace usage of removed openstack.version https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992598 | 10:35 |
| noonedeadpunk | seems I was able to replace packaging with regexp... | 10:38 |
| opendevreview | Dmitriy Rabotyagov proposed openstack/ansible-collections-openstack master: Replace usage of removed openstack.version https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992598 | 10:39 |
| opendevreview | Dmitriy Rabotyagov proposed openstack/ansible-collections-openstack master: Replace usage of removed openstack.version https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992598 | 10:41 |
| noonedeadpunk | mnasiadka: talking about releases - should I make a new "bugfix" release for ovs? | 10:53 |
| opendevreview | Michal Arbet proposed openstack/ansible-collections-openstack master: Use importlib.metadata for openstacksdk version detection https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992613 | 10:56 |
| noonedeadpunk | this one actually looks good and we can probably merge it right away ^ | 11:02 |
| noonedeadpunk | and I can just add a regexp here as a follow-up | 11:03 |
| noonedeadpunk | mnasiadka: ^ | 11:03 |
| noonedeadpunk | but folks, you have some interesting contribution culture I must admin... | 11:06 |
| opendevreview | Dmitriy Rabotyagov proposed openstack/ansible-collections-openstack master: Ensure dev versions of SDK are parsed properly https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992616 | 11:15 |
| opendevreview | Michal Arbet proposed openstack/ansible-collections-openstack master: Use importlib.metadata for openstacksdk version detection https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992613 | 11:18 |
| noonedeadpunk | *admit | 11:24 |
| mnasiadka | We can merge once it passes Zuul - I’ll have a look later | 11:27 |
| opendevreview | Dmitriy Rabotyagov proposed openstack/ansible-collections-openstack master: Revive collection unit tests https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992621 | 11:39 |
| noonedeadpunk | I made also unit tests to cover my patch and revive them in general - but I can rebase it on whatever merges, not a problem | 11:42 |
| opendevreview | Stephen Finucane proposed openstack/ansible-collections-openstack master: Remove version checks https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992623 | 11:44 |
| opendevreview | Stephen Finucane proposed openstack/ansible-collections-openstack master: Remove sdk_version extra var https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992624 | 11:44 |
| noonedeadpunk | lol | 11:44 |
| noonedeadpunk | :D | 11:44 |
| opendevreview | Stephen Finucane proposed openstack/ansible-collections-openstack master: Remove sdk_version extra var https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992624 | 11:46 |
| opendevreview | Stephen Finucane proposed openstack/ansible-collections-openstack master: Remove version checks https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992623 | 11:46 |
| noonedeadpunk | fwiw, my patch (https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992598) has just passed, and unit tests in follow-up are also passing. | 12:11 |
| noonedeadpunk | but I think I like Stephen's approach way more. but it's failing badly... | 12:11 |
| noonedeadpunk | this one also look good enough: https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992613 - I don't mind if we merge it as we can get votes there faster | 12:12 |
| *** gmaan_ is now known as gmaan | 12:24 | |
| mnasiadka | noonedeadpunk: well, you need to decide on something ;-) | 12:40 |
| noonedeadpunk | I like the fact of unit tests being around :) But I can rebase them - it's not an issue. | 12:47 |
| noonedeadpunk | I am not sure I can dig into why Stephen's patch is failing though | 12:48 |
| noonedeadpunk | So while clean-up is great, I'd say we can do it in a more planned manner then urgent one | 12:48 |
| noonedeadpunk | unelss indeed squashing is the way forward | 12:49 |
| noonedeadpunk | as dropping this logic alltogether is the real solution :) | 12:49 |
| opendevreview | Stephen Finucane proposed openstack/ansible-collections-openstack master: Remove version checks https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992623 | 13:06 |
| mnasiadka | noonedeadpunk: I think https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992623 might be the best fix | 13:51 |
| noonedeadpunk | ++ | 13:52 |
| noonedeadpunk | agree | 13:52 |
| noonedeadpunk | I am just waiting for CI to pass | 13:52 |
| noonedeadpunk | And quite happy we didn't rush with solution tbh | 13:56 |
| noonedeadpunk | and it failed 2.18.... | 14:42 |
| noonedeadpunk | likely intermittently | 14:42 |
| mnasiadka | Yeah, cinder lvm setup in devstack has some weird problem sometimes | 15:10 |
| mnasiadka | noonedeadpunk: passed now, merging | 16:18 |
| opendevreview | Dmitriy Rabotyagov proposed openstack/ansible-collections-openstack master: Revive collection unit tests https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992621 | 17:08 |
| opendevreview | Dmitriy Rabotyagov proposed openstack/ansible-collections-openstack master: Revive collection unit tests https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992621 | 17:09 |
| noonedeadpunk | once it merge I can rebase https://review.opendev.org/c/openstack/ansible-collections-openstack/+/983021 | 17:15 |
| noonedeadpunk | as eventually we need to make a release note for this fix | 17:24 |
| noonedeadpunk | I bet ppl will be looking for it... | 17:24 |
| opendevreview | Merged openstack/ansible-collections-openstack master: Remove version checks https://review.opendev.org/c/openstack/ansible-collections-openstack/+/992623 | 17:28 |
| opendevreview | Dmitriy Rabotyagov proposed openstack/ansible-collections-openstack master: Prepare release metadata for openstack.cloud 2.6.0 https://review.opendev.org/c/openstack/ansible-collections-openstack/+/983021 | 17:37 |
| opendevreview | Dmitriy Rabotyagov proposed openstack/ansible-collections-openstack master: Prepare release metadata for openstack.cloud 2.6.0 https://review.opendev.org/c/openstack/ansible-collections-openstack/+/983021 | 17:37 |
| noonedeadpunk | mnasiadka: here we go ^ - I added minimalistic note for sdk compatability | 17:38 |
| opendevreview | James Hewitt proposed openstack/ansible-collections-openstack master: Add support for NDP proxies https://review.opendev.org/c/openstack/ansible-collections-openstack/+/972496 | 17:39 |
| mnasiadka | noonedeadpunk: merging, we need to push a signed tag for the release | 18:47 |
| noonedeadpunk | yup | 19:21 |
| noonedeadpunk | oh, it's not managed with releases.... | 19:21 |
| noonedeadpunk | Ok, I guess I have no idea how to do that then.... | 19:22 |
| opendevreview | Merged openstack/ansible-collections-openstack master: Prepare release metadata for openstack.cloud 2.6.0 https://review.opendev.org/c/openstack/ansible-collections-openstack/+/983021 | 20:03 |
| mnasiadka | Ok, 2.6.0 tag pushed | 23:18 |
Generated by irclog2html.py 4.1.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!