| *** ysandeep|out is now known as ysandeep | 01:14 | |
| *** dviroel|afk is now known as dviroel | 01:16 | |
| *** dviroel is now known as dviroel|out | 01:22 | |
| *** ysandeep is now known as ysandeep|afk | 01:40 | |
| opendevreview | Ian Wienand proposed zuul/zuul-jobs master: ensure-virtualenv: Don't support on CentOS 9-stream https://review.opendev.org/c/zuul/zuul-jobs/+/827589 | 02:28 |
|---|---|---|
| opendevreview | Ian Wienand proposed zuul/zuul-jobs master: ensure-virtualenv: Don't support on CentOS 9-stream https://review.opendev.org/c/zuul/zuul-jobs/+/827589 | 03:45 |
| *** akahat is now known as akahat|rover | 05:29 | |
| *** ysandeep|afk is now known as ysandeep | 07:09 | |
| ianw | finally got the quotes right | 07:26 |
| ianw | (on 589) | 07:26 |
| *** amoralej|off is now known as amoralej | 07:28 | |
| frickler | ianw: are we sure that c9 and c9s will be identical in that regard? because you talk about c9s but only test for centos==9 | 07:38 |
| ianw | frickler: aiui, there will not be a 9, and 9-stream reports itself as "9" (without the stream) | 08:12 |
| ianw | similar issue with $releasever set in https://review.opendev.org/c/openstack/diskimage-builder/+/826244 | 08:14 |
| rpittau | good morning! I was talking the other day with fungi and clarkb about an issue with debian bullseye DIB images not getting ips from dhcp, I have to rectify myself, we don't use simple-init for the ipa ramdisk, we just use dhcp-all-interfaces | 08:14 |
| rpittau | I'm able to reproduce the issue locally, it looks like the interfaces with the predictable names are completely ignored | 08:14 |
| rpittau | using net.ifnames=0 won't work as we pass kernel args by pxe, not by grub, also I guess this should just work without the need to do that | 08:14 |
| *** akahat|rover is now known as akahat|lunch | 08:30 | |
| *** ysandeep is now known as ysandeep|lunch | 08:36 | |
| *** akahat|lunch is now known as akahat|rover | 09:22 | |
| mnasiadka | Good morning - is there a way to map a tarball from tarballs.opendev.org (e.g. $project-master.tar.gz) to a git commit hash? I can't seem to find any metadata in the tarball itself, that would help in that matter. | 10:00 |
| *** ysandeep|lunch is now known as ysandeep | 10:04 | |
| *** pojadhav is now known as pojadhav|brb | 10:24 | |
| *** dviroel|out is now known as dviroel|ruck | 10:46 | |
| *** rlandy|out is now known as rlandy|ruck | 11:10 | |
| *** pojadhav|brb is now known as pojadhav | 11:14 | |
| *** ysandeep is now known as ysandeep|mtg | 11:17 | |
| opendevreview | Riccardo Pittau proposed openstack/diskimage-builder master: Move dhcp script to correct custom dir https://review.opendev.org/c/openstack/diskimage-builder/+/827846 | 11:27 |
| *** ysandeep|mtg is now known as ysandeep | 12:02 | |
| fungi | mnasiadka: if the project uses pbr, then the hash is in the package metadata. you can use the `pbr freeze` subcommand to see it, or access the pbr.json metadata file directly | 13:29 |
| frickler | fungi: we're not talking about wheels, but source tarballs | 13:48 |
| frickler | although there's a PKG-INFO in there, with e.g. "Version: 13.1.0.dev51", maybe that's already enough | 13:51 |
| frickler | mnasiadka: ^^ | 13:51 |
| mnasiadka | 51 commits from 13.1.0? well, it's a pointer, but as a Kolla user probably I wouldn't be the most happy guy on earth :) | 13:52 |
| *** pojadhav- is now known as pojadhav | 13:53 | |
| frickler | mnasiadka: well, it should be unique unless you have local patches, and in that case it is more helpful than 8966e65d probably. if you don't know in which repo+branch to look for it, the latter won't likely help you much, either | 13:58 |
| *** ysandeep is now known as ysandeep|mtg | 13:59 | |
| mnasiadka | frickler: just trying to think how best to "find which point of Neutron stable/wallaby code I have running" | 13:59 |
| fungi | frickler: pbr puts its medatadata in sdist tarballs too | 14:13 |
| frickler | fungi: seemingly only the version I mentioned above, not the git sha | 14:19 |
| frickler | and if I do "pbr info designate" on an install from the tarball, it is also missing that part as compared to when installing from git directly | 14:20 |
| fungi | huh, i wonder if we stopped stashing the metadata in there and didn't notice | 14:21 |
| frickler | fungi: that seems to be the case, stable-ocata contains designate.egg-info/pbr.json, master tarball doesn't | 14:25 |
| fungi | yeah, i have a feeling something changed in setuptools/pip causing us to silently stop including it. i'll dig deeper | 14:25 |
| fungi | a bit of spelunking in one of my personal projects which always uses the latest of everything was including a pbr.json in sdist tarballs as recently as august of last year | 14:42 |
| fungi | going to see if i can narrow down the timeframe any better than that | 14:42 |
| fungi | but that indicates it's been a fairly recent regression, at least | 14:42 |
| fungi | makes me wish i'd been releasing that project early and often so i'd have a tighter window on it | 14:45 |
| *** ysandeep|mtg is now known as ysandeep|out | 14:53 | |
| frickler | fungi: checking designate tarballs, https://tarballs.opendev.org/openstack/designate/designate-13.0.0.tar.gz from 2021-10-06 is good, stable tarballs from 2021-12-02 are not, so that's 2 months timeframe | 14:56 |
| fungi | thanks, that gives me a much smaller number of pbr, setuptools and pip versions to check | 14:58 |
| fungi | assuming we used latest of those at the time. i might need to factor tox/virtualenv versions into that since they can influence what version of setuptools is used | 14:58 |
| opendevreview | Merged openstack/project-config master: Add cirros/cirros project https://review.opendev.org/c/openstack/project-config/+/827719 | 14:59 |
| frickler | kolla has some rc releases in that timeframe, I'm going to check those, too | 14:59 |
| frickler | o.k., so that looks like the change happened between 2021-11-05 and 2021-11-19, kolla 13.0.0.0rc2 vs 13.0.0.0rc3. but of course it may have been a change in our tooling rather than upstream versions | 15:02 |
| fungi | right, but that's only two weeks to check at least. i'll be able to dig deeper after the d&i wg meeting is over | 15:07 |
| frickler | https://opendev.org/cirros/cirros looks as expected, is there anything specific we wanted to check in term of the main branch? I'm going to add myself to cirros-core now and see if there's a small job I can set up other than noop | 15:11 |
| fungi | yeah, zuul job configuration was the thing we're needing to test | 15:13 |
| *** dviroel|ruck is now known as dviroel|ruck|lunch | 15:17 | |
| Clark[m] | fungi: if I had to guess setuptools stopped calling the hook that generates that file on sdist but does call it for wheels | 15:41 |
| fungi | Clark[m]: it's not happening for wheels either though | 15:41 |
| fungi | from what i can see | 15:41 |
| Clark[m] | Oh I missed that. Maybe they dropped the hook entirely then? | 15:42 |
| Clark[m] | I guess bisect would hopefully figure it out if it was a pbr change pretty easily but harder if new setuptools behavior | 15:43 |
| *** akahat|rover is now known as akahat|dinner | 15:45 | |
| frickler | so this looks good to me, gonna step away for a bit and then try to do a real job https://review.opendev.org/c/cirros/cirros/+/827878 | 15:47 |
| fungi | okay, my meetings are done for the day. i've got a couple other quick things i need to knock out, and then i can start looking more closely at the pbr metadata problem | 16:18 |
| *** dviroel|ruck|lunch is now known as dviroel|ruck | 16:22 | |
| *** rlandy|ruck is now known as rlandy|ruck|brb | 16:30 | |
| *** marios is now known as marios|out | 16:38 | |
| *** rlandy|ruck|brb is now known as rlandy|ruck | 16:54 | |
| clarkb | I'm going to try and catch on reviews in various places today as I have to pop out midday for an appointment | 17:13 |
| clarkb | Don't want toget too deep into something I can't put down :) | 17:13 |
| fungi | sounds good. i've rolled up my sleeves and am trying to bisect pbr versions first | 17:14 |
| fungi | this should go pretty quickly. pbr 5.7.0 is working and 5.8.0 is not, with everything else the same in the venv (simply downgraded pbr), so i'm pretty sure it's going to turn out to be a pbr regression | 17:15 |
| fungi | there are only 3 changes between them | 17:16 |
| clarkb | interesting. I wonder if it related to the explicit avoidance of the recursion that the pep XYZ (whatwas the number?) had to do | 17:17 |
| fungi | one only changes zuul jobs | 17:17 |
| clarkb | since that could potentially cahnge what hooks run maybe? | 17:17 |
| fungi | and one only changes tests | 17:18 |
| clarkb | What is odd if that is the case was that traces showed that is how it worked when run normally not via the new pep interface. Definitely will be a fun thing to debug if that is the case | 17:18 |
| fungi | so yes, i think it has to be https://review.opendev.org/817794 "Allow PEP517 without setup_requires" | 17:18 |
| fungi | i'll revert that commit on top of 5.8.0 and see if i get the expected metadata file injected again | 17:19 |
| fungi | okay, not as straightforward as i expected. now upgrading back to pbr 5.8.0 in the venv and things are working | 17:24 |
| fungi | i probably have to recreate the venv, may be something to do with editable installs | 17:24 |
| opendevreview | Merged zuul/zuul-jobs master: ensure-virtualenv: Don't support on CentOS 9-stream https://review.opendev.org/c/zuul/zuul-jobs/+/827589 | 17:24 |
| fungi | interesting, updating to pbr with the revert in that venv, things are still broken | 17:27 |
| fungi | suggesting it may not be that change, or that simply reverting it atop master is not sufficient | 17:28 |
| clarkb | neat. I mean when I wrote that chnage I thought I was pretty confident we were preserving the old behavior based on traces of what was going on. So I'm surprised it broke, but I also think that must be the only change that could do it? | 17:29 |
| fungi | yeah, which makes me suspect my test methodology is off | 17:29 |
| fungi | need to simplify it a bit | 17:29 |
| fungi | okay, simpler testing for the win | 17:33 |
| fungi | yes, reverting that change on top of master gets me a gear-0.16.0/gear.egg-info/pbr.json when building gear in a clean checkout with a fresh venv with that modified pbr preinstalled | 17:34 |
| fungi | same as for pbr==5.7.0 preinstalled | 17:34 |
| fungi | with pbr==5.8.0 preinstalled the pbr.json file is missing from both the sdist and wheel | 17:34 |
| fungi | so i'm confident the regression is there | 17:35 |
| fungi | now to revisit how pbr expects to install its metadata file | 17:35 |
| fungi | write_pbr_json has a couple of bailouts to short-circuit at start | 17:38 |
| fungi | if not hasattr(cmd.distribution, 'pbr') or not cmd.distribution.pbr | 17:38 |
| fungi | if not git._run_git_functions() | 17:38 |
| clarkb | hrm that first one might be a problem if we want to setting pbr=True but I don't think that has been in the cards yet | 17:38 |
| fungi | if git.get_git_short_sha(git_dir) is not None | 17:38 |
| fungi | failing any of those would cause it not to write the file | 17:39 |
| fungi | i can inject some debug prints in there and see what's happening | 17:39 |
| clarkb | *if we want to stop setting pbr=True | 17:40 |
| *** amoralej is now known as amoralej|off | 17:41 | |
| fungi | cmd.distribution.pbr is None | 17:45 |
| clarkb | huh even though pbr=True is set ? | 17:45 |
| fungi | apparently | 17:45 |
| clarkb | I wonder if we're sideffecting ourselves then. Because the entire pbr system relies on pbr=True setting that over | 17:46 |
| clarkb | that is what calls the main pbr entrypoint | 17:46 |
| fungi | that's where it's bailing out of pbr.pbr_json.write_pbr_json() | 17:46 |
| clarkb | oh! | 17:46 |
| fungi | could it be evaluating a copy? | 17:46 |
| clarkb | that change sets pbr to None after we are done evaluating it or we thought we were done | 17:46 |
| fungi | hah! | 17:47 |
| clarkb | fungi: the reason we were looping is that without the setup_requires entry setuptools kept calling the pbr method because pbr was True | 17:47 |
| clarkb | I didn't fully understand why that was but set it to None to short circuit the recursion | 17:47 |
| clarkb | fungi: I'm thinking that maybe we need to rely on a PBR internal flag rather than setuptools flags once we get out of the main entrypoint | 17:47 |
| clarkb | basically separate the setuptools state from pbr state with a trnslation layer as necessary in the main entrypoint? | 17:48 |
| fungi | i'll need to stare at it a bit more to grok what you're saying | 17:48 |
| clarkb | no worries. I find I have to do that and page a bunch of context in every time too :/ | 17:48 |
| fungi | the commit message on the change has a pretty good explanation as to why it's setting it to none | 17:49 |
| fungi | okay, so you're saying we should have pbr itself bail before calling finalize_options if we've already called it once? | 17:51 |
| fungi | i wonder what option we can persist that tracking flag in | 17:52 |
| clarkb | fungi: I'm not quite sure of that. We want to call finalize options but then not have setuptools call the pbr entrypoint recursively through that | 17:52 |
| fungi | er, what object | 17:52 |
| clarkb | If we can attach a flag like pbr_in_use to something we can check that rather than distribution.pbr | 17:52 |
| clarkb | and continue to set pbr to None at the end of the entrypoint before recursion may occur | 17:53 |
| fungi | yeah, i just need to dig around a bit more to see what objects we can expect to be resident | 17:53 |
| fungi | i guess we could try attaching a separate falsey attribute to dist and toggling on its presence rather than its value | 17:55 |
| fungi | add the attribute on first call, skip if the attribute is already there? | 17:56 |
| fungi | seems dirty though since it's not an object pbr itself instantiates | 17:56 |
| clarkb | ya I think distribute/setuptools infer stuff about those attributes too in order to call entrypoints | 17:57 |
| clarkb | I'm not sure if that will lead to errors if the entrypoint isn't present. Or if there ends up being a collision unexpected behavior | 17:57 |
| fungi | right | 17:57 |
| clarkb | can we just set a global module flag? | 17:57 |
| fungi | we could, but is that reentrant? | 17:58 |
| clarkb | I don't think the reentrancy matters here since this flag would flip from false to True once and then never change | 17:58 |
| fungi | even if something else imports pbr, yeah i guess the import is a noop so long as it's not a reload | 17:59 |
| clarkb | fungi: Don't change the check for pbr and flipping it to None in the entrypoint. Instead set pbr.core.in_use to True at the beginning of the entrypoint and then have the json writer check that value instead of the dist value | 17:59 |
| *** akahat|dinner is now known as akahat|rover | 17:59 | |
| fungi | "the entrypoint" being pbr.build? | 18:00 |
| clarkb | fungi: pbr.core.pbr | 18:01 |
| clarkb | I believe that is the top level entrypoint into pbr | 18:01 |
| fungi | oh, that entrypoint, okay | 18:01 |
| clarkb | since setuptools looks for a method called pbr when you set pbr=True in its setup() arglist | 18:01 |
| fungi | yeah, i thought you meant the pep 417 build backend entrypoint | 18:01 |
| clarkb | and in that we set pbr to None to prevent recursion. Which causes the json writing to noop | 18:02 |
| fungi | 517 | 18:02 |
| clarkb | but I think we can address that by having the json writer check for a different value and not modify pbr() setting of dist.pbr to None | 18:02 |
| clarkb | Because if we don't stop setting dist.pbr to None it does infinite recursion through pep 517 without setup_requires | 18:02 |
| clarkb | (I don't undersatnd why, we could potentially debug that behavior morefully and come up with a better answer, but I spent a bit of time trying to undersatnd that before giving up) | 18:03 |
| fungi | i guess i need to declare in_use as a global in pbr.core? | 18:10 |
| clarkb | I think you do when accessing it in a method otherwise it assumes scope local if not scoped | 18:10 |
| fungi | it's not really a method so much as a function in this case, but yeah trying | 18:11 |
| fungi | bingo! | 18:12 |
| fungi | i'll strip out the debug prints, add some code comments, and see if i can come up with a regression test | 18:12 |
| fungi | i'll also test it in a pep-517 using project (i've been testing with gear so far) | 18:13 |
| clarkb | ++ | 18:15 |
| fungi | mmm, pep-517 build isolation makes this harder | 18:22 |
| fungi | to test | 18:22 |
| fungi | the installed pbr doesn't get used | 18:22 |
| fungi | trying to set the path to my modified pbr in pyproject.toml instead | 18:24 |
| clarkb | fungi: in the existing tests for pep517 we use build beacuse you can turn off isolation | 18:24 |
| clarkb | fungi: that allows us to preinstall what we want and test without being overriddent from pypi | 18:25 |
| fungi | yeah, but i want to make sure isolation doesn't break pbr | 18:25 |
| clarkb | aha | 18:25 |
| fungi | writing pbr to mudpy.egg-info/pbr.json | 18:25 |
| fungi | yep, working! | 18:26 |
| fungi | pbr.json is showing up in both the sdist and wheel | 18:27 |
| fungi | okay, so just need to comment this up and figure out how to test it | 18:27 |
| fungi | in zuul | 18:27 |
| clarkb | fungi: Ithink you should be able to take the existing tests that install a package and inspect the metadata pbr.json for sha content | 18:28 |
| clarkb | shouldn't need new tests, just update a coupel of the existing tests to ensure that data is making it into the final product? | 18:28 |
| fungi | yeah, that could work. thanks | 18:30 |
| fungi | i'll squash the test into the fix once zuul confirms both are working | 18:41 |
| clarkb | fungi: left a note on the fix change | 18:43 |
| clarkb | a small simplification | 18:43 |
| fungi | clarkb: yeah, i originally had it initialize to false but then realized we had the hasattr check making it unnecessary. potato/potato i guess | 18:46 |
| fungi | more pythonic would be to just check the attr directly and then catch the exception raised when it's absent | 18:47 |
| fungi | but pbr has a lot of lbyl style so i tried to mimic what's there | 18:47 |
| clarkb | wfm | 18:52 |
| clarkb | for flags we know we always want to check I alawys like to set them up | 18:52 |
| fungi | yeah, if you prefer it that way, i'm happy to initialize it first | 18:55 |
| clarkb | I'm fine either way. Thank you for looking into this and fixing it | 18:56 |
| fungi | well, it seems relatively urgent, because we're publishing releases with no git metadata in them at the moment | 18:56 |
| fungi | once we're good with the fix and regression test, i'll see if we can expedite a release | 18:56 |
| fungi | huh, the fix isn't fixing the test (aside from the fact that the test is checking for the wrong list item. the test package doesn't seem to include a pbr.json | 19:10 |
| clarkb | I wonder if it is using the wrong version of pbr because setup_requires | 19:11 |
| clarkb | fungi: I think that might be happening | 19:12 |
| fungi | so the other tests there are also bunk in that case | 19:13 |
| clarkb | if you look in test_packaging there is much more robust virtualenv setup and installtion of pbr to the code under test | 19:13 |
| clarkb | but the test you modified is relying on the base test class which seems far more rudimentary | 19:13 |
| fungi | yeah, i have a feeling that existing test was useless | 19:14 |
| fungi | okay, managed to get that working locally | 19:34 |
| clarkb | I went ahead and +2'd both changes since I'm not sure I'll be around when CI results come back. I think landing that quickly is worthwhile | 19:39 |
| clarkb | feel free to reapply my +2 postsquash | 19:39 |
| fungi | thanks! | 19:40 |
| clarkb | fungi: looks like unittests are still failing? | 19:48 |
| clarkb | you may need to instrument those cases and check that pbr is what you expect it to be? | 19:48 |
| fungi | huh, yeah looks like it's failing pbr.tests.test_pbr_json.TestJsonContent.test_content because write_file wasn't called? | 19:57 |
| Clark[m] | I strongly suspect that it just isn't getting an updated pbr. Maybe yolo some print statements in there to see | 19:58 |
| Clark[m] | In pbr() if you print it should show up if new pbr is used | 19:58 |
| fungi | well, that's not the test i added, it's an existing test | 19:58 |
| fungi | so presumably my change broke it | 19:58 |
| Clark[m] | Oh I see | 19:59 |
| Clark[m] | Huh | 19:59 |
| fungi | reprodicible locally too, so i'll see if i can work out what's gone wrong | 20:00 |
| fungi | oh! | 20:02 |
| fungi | the test is calling directly into pbr_json.write_pbr_json() and so our global flag from the entrypoint is never set | 20:03 |
| fungi | i guess the test could set that flag, i'll try it | 20:03 |
| Clark[m] | Aha | 20:03 |
| fungi | yep, that'll work | 20:05 |
| fungi | well, it's passing more jobs now, at least. not all | 20:31 |
| fungi | https://zuul.opendev.org/t/openstack/build/64430af0703341abab1789d27d0e8636 looks like some sort of transitive import problem specific to python 2.7 | 20:33 |
| fungi | recursive import maybe? | 20:34 |
| fungi | util->hooks->hooks.backwards->packaging->pbr_json->core->util | 20:35 |
| Clark[m] | Ya looks like it is a loop that it is complaining about | 20:36 |
| fungi | the pbr_json->core is what my patch added | 20:36 |
| fungi | py3x seems to take it in stride | 20:36 |
| Clark[m] | Maybe we can put the flag in until.py to avoid that? | 20:37 |
| Clark[m] | Hrm maybe not | 20:37 |
| Clark[m] | But I'm no longer at a computer so hard to think through it | 20:37 |
| fungi | yeah, i'll noodle on it | 20:37 |
| fungi | if we put it in util then we still have the cycle, it just closes one import sooner | 20:38 |
| fungi | since pbr_json will need to import util directly instead of importing core | 20:38 |
| fungi | looks like the problem might be specific to the "from foo import bar" construct | 20:40 |
| fungi | okay, if i replace the "from pbr import util" in pbr.core and switch its util calls to pbr.util then the circular import problem does seem to go away | 20:43 |
| fungi | i'll propose that as a separate patch just to make sure we're not papering over anything in the other one | 20:45 |
| fungi | hrm, no, pbr.core defines a pbr function locally, which ends up shadowing the pbr module. this is a mess | 21:20 |
| fungi | er, shadowing the pbr package | 21:21 |
| fungi | this will need more thought | 21:21 |
| * fungi already despised relative imports, but now hates them even more | 21:21 | |
| Clark[m] | I wonder if we can just assume if that method is called that pbr is in use | 21:21 |
| Clark[m] | Drop the flag entirely I mean | 21:21 |
| fungi | worth a try | 21:22 |
| fungi | i'll rejigger all this and see what i get | 21:22 |
| Clark[m] | Off the top of my head I'm not sure what context it would be called in where we don't want the file to be generated | 21:22 |
| fungi | right, it was probably a safeguard against it being called when there's no dist to process | 21:34 |
| clarkb | hrm like when doing a setup.py othercommand? | 21:38 |
| fungi | i doubt we'd actually call into the module in those situations, but i guess testing will prove whether it's safe | 21:38 |
| clarkb | seems reasonable | 21:39 |
| clarkb | fungi: I suspect that it may be because pbr.json is setup as an egg info writer entrypoint | 21:42 |
| clarkb | so if you've got pbr installed but are doing a non pbr package you need it to noop? | 21:42 |
| clarkb | This raelly is a mess :( | 21:42 |
| fungi | oh, yeah so for building non-pbr packages when pbr is installed in the environment | 21:43 |
| clarkb | ya | 21:43 |
| clarkb | and that explains why this broke in the first place since it doesn't get invoked via the pbr() entrypoint | 21:43 |
| clarkb | its a separate entrypoint after pbr is processed | 21:43 |
| fungi | so we need some way to identify whether the dist for which this is being called is a "pbr" type dist | 21:44 |
| clarkb | fungi: maybe we can get away with hasattr pbr | 21:44 |
| fungi | and we can't rely on the presence of the pbr entrypoint for the dist because we need to turn it off after | 21:44 |
| fungi | oh, right! | 21:44 |
| fungi | testing | 21:44 |
| clarkb | since I believe that is only set if you put a pbr=Something flag in a setup.py setup() call | 21:45 |
| clarkb | I suppose you may set it to False or None explicitly though and then that gets weird | 21:45 |
| clarkb | attrs = util.cfg_to_args(path, dist.script_args) <- that is the only thing in util that pbr.core.pbr needs | 21:47 |
| clarkb | We might be able to split that into its own separate thing to break the loop | 21:48 |
| clarkb | hrm but that ends up calling a bunch of other stuff in util | 21:49 |
| clarkb | Another approach would be to track entries into pbr() and return if >1 | 21:50 |
| clarkb | then leave dist.pbr = True | 21:50 |
| fungi | oh, like have core exit with extreme prejudice if the in_use global is true? | 21:51 |
| fungi | i can try that next | 21:51 |
| clarkb | ya basically only check the in_use flag in core.pbr() and then avoid any import trouble by having the tendril code look at dist.pbr | 21:52 |
| clarkb | and remove the switch to None. That mgith end up being the cleanest thing | 21:52 |
| clarkb | As ugly as it is it preserves the existing check we knew worked. And should avoid the recursion that we're trying to avoid | 21:53 |
| clarkb | fungi: I just pushed the counter idea if you want to look at it and see if that seems good. Feel free to edit or replace too | 21:58 |
| fungi | i also pushed the one we were just discussing to see if it will work | 21:59 |
| fungi | local spot checks of some tests i was worried about came out clean at least | 21:59 |
| fungi | i need to context-switch to dinnermaking now, but will check on things once i'm done eating | 21:59 |
| clarkb | ya yours is basically what i did. I think it is worth writing down in the new comments where the source of recursion is happening. I tried to do that in my change but I'm hapyp with either one or to have you update your change to update the comments | 22:00 |
| clarkb | and ya thinking about it maybe a binary flag is better than a coutner since we only care if not 0 count | 22:00 |
| clarkb | I'll leave a note about improving comments on yours. enjoy dinner! | 22:01 |
| fungi | thanks! | 22:03 |
| *** dviroel|ruck is now known as dviroel|out | 22:06 | |
| clarkb | fungi: testing on the recursion fixup change looks really good. I think that is the one to go with. Just need to update the comment, squash into the test and then merge? | 23:14 |
| fungi | yep! | 23:18 |
| opendevreview | Clark Boylan proposed opendev/system-config master: Install Limnoria from upstream https://review.opendev.org/c/opendev/system-config/+/821331 | 23:22 |
| clarkb | maybe next week I'll actually find time to land that when meetings aren't happenign and I can double check stuff | 23:22 |
| clarkb | cool https://review.opendev.org/c/openstack/pbr/+/827931 got a +1 so ya I think we can add more text to the comment and then squash in the test change and send it | 23:29 |
| fungi | yeah, working on it now | 23:33 |
| fungi | clarkb: updated, thanks! | 23:36 |
| clarkb | I went ahead and approved it. Thanks. I think others can chime in between now and whenever a release is made if they want changes made | 23:40 |
| fungi | thanks! | 23:40 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!