*** Syed__ has quit IRC | 00:45 | |
*** pwnall1337 is now known as zz_pwnall1337 | 00:52 | |
openstackgerrit | Merged openstack/craton master: Pretty-print format all JSON response bodies https://review.openstack.org/442588 | 01:33 |
---|---|---|
*** VW has joined #craton | 02:42 | |
*** VW has quit IRC | 03:01 | |
*** VW has joined #craton | 03:02 | |
openstackgerrit | Jim Baker proposed openstack/craton master: Updates Alembic migration to better match SQLAlchemy models https://review.openstack.org/441644 | 04:19 |
*** openstackgerrit has quit IRC | 09:03 | |
*** VW_ has joined #craton | 10:17 | |
*** VW has quit IRC | 10:17 | |
sulo | jimbaker: thomasem: i left more comments on this patch: | 10:24 |
sulo | https://review.openstack.org/#/c/441644/ | 10:24 |
*** david-lyle has quit IRC | 12:30 | |
*** david-lyle has joined #craton | 12:33 | |
sigmavirus | morning | 13:01 |
sulo | sigmavirus: morning | 13:04 |
*** VW_ has quit IRC | 13:16 | |
sulo | btw i took this bug: https://bugs.launchpad.net/craton/+bug/1670561 | 13:18 |
openstack | Launchpad bug 1670561 in craton "Create bootstrap user and project only via SQLAlchemy models" [Critical,New] - Assigned to Sulochan Acharya (sulochan-acharya) | 13:18 |
sulo | thomasem: ^ i know you've been meaning to look at it as well ... so fyi | 13:18 |
thomasem | sulo: Thanks for the heads up!! | 13:35 |
*** openstackgerrit has joined #craton | 13:39 | |
openstackgerrit | Tomi Juvonen proposed openstack/craton master: docker install documentation not consistent https://review.openstack.org/443129 | 13:39 |
*** VW has joined #craton | 13:52 | |
thomasem | Here's the bug for JSONPath querying: https://bugs.launchpad.net/craton/+bug/1671116 | 14:17 |
openstack | Launchpad bug 1671116 in craton "Lacking support for searching nested variables" [High,New] - Assigned to Thomas Maddox (thomas-maddox) | 14:17 |
thomasem | I would appreciate folks' thoughts on that one. | 14:20 |
thomasem | I'm thinking it'll make sense to modify our data model to bring the variable keys down one layer and make it simply (id, _value) where _value is a JSON column. | 14:21 |
thomasem | Otherwise, the existing potential for a document or a string won't work. | 14:21 |
thomasem | in the _value column | 14:22 |
thomasem | So, where we'd have (1, "name", "value") or (2, "name", '{"foo": "bar"}'), we'd instead have (1, '{"name": "value"}') and (2, '{"name": {"foo": "bar"}}') | 14:23 |
thomasem | FYI, this is especially because MySQL will validate that it's valid JSON being stored in that column, which makes sense. | 14:24 |
thomasem | especially painful* :) | 14:25 |
fsaad | morning guys | 14:36 |
fsaad | thomasem: thanks for the jsonpath query bug link | 14:37 |
fsaad | zz_pwnall1337: ^^ https://bugs.launchpad.net/craton/+bug/1671116 | 14:37 |
openstack | Launchpad bug 1671116 in craton "Lacking support for searching nested variables" [High,New] - Assigned to Thomas Maddox (thomas-maddox) | 14:37 |
thomasem | fsaad: you bet! | 14:59 |
*** VW has quit IRC | 15:01 | |
*** VW has joined #craton | 15:02 | |
jimbaker | sulo, re https://review.openstack.org/#/c/441644/, i only rebased that patch. the users FK change in the alembic should stand, since the model is what it is and that's a requirement of the model | 15:16 |
jimbaker | but i can remove NetworkInterface mixin of VariableMixin | 15:16 |
jimbaker | we almost certainly want the project dependency to prevent dangling; but this is related to https://bugs.launchpad.net/craton/+bug/1668081, so i think it's best to combine in the tests that demonstrate the fix in with this patch | 15:18 |
openstack | Launchpad bug 1668081 in craton "500 on resource deletes from foreign key constraint error" [Undecided,New] | 15:18 |
jimbaker | sounds good? | 15:18 |
thomasem | Actually, I'm wrong. It looks like SQLAlchemy converts the string to valid JSON. | 15:18 |
thomasem | Handy. Albeit a bit awkward, still. | 15:18 |
thomasem | I still think it makes more sense to push the key_ down into a JSON document, but I can truck forward with this, I think. | 15:19 |
openstackgerrit | Sulochan Acharya proposed openstack/craton master: Adds project/user bootstrap command to dbsync https://review.openstack.org/443170 | 15:21 |
sulo | jimbaker: rgr, also if you want to wait, i can finish up this (not ready yet) and my concerns around | 15:21 |
sulo | having to do manual db stuff will be taken care of too | 15:21 |
thomasem | https://gist.github.com/thomasem/8649f8e0b07573cfb82975fba2930fd9 | 15:22 |
jimbaker | sulo, exactly - we don't want people using raw SQL against the DB. or even model code :) | 15:22 |
thomasem | Except that guy Thomas. | 15:22 |
thomasem | He can. | 15:22 |
* thomasem snickers | 15:23 | |
jimbaker | thomasem, oh sure, we always let thomas do what he wants | 15:23 |
sulo | jimbaker: that patch is mostly taking shape .. i need to update docs etc .. and if y'all can give it a looksee to see everyone is happy with how that command is looking | 15:23 |
sulo | i am talking about https://review.openstack.org/#/c/443170/ | 15:23 |
jimbaker | cool, will take a look | 15:23 |
thomasem | sulo: added a couple comments to that review regarding setting up variable association reference for those. | 15:27 |
jimbaker | thomasem, same here, but with regards to API key | 15:28 |
sulo | thomasem: huh .. i think vars association gets taken care of auto | 15:28 |
thomasem | It does? | 15:28 |
* sulo jumps into the db to check | 15:29 | |
thomasem | I thought it didn't and that's why I had to add https://github.com/openstack/craton/blob/master/craton/tests/functional/__init__.py#L172 | 15:29 |
jimbaker | sulo, thomasem, it does | 15:29 |
sulo | yeah it does | 15:29 |
sulo | thomasem: i think thats only if you insert records | 15:29 |
jimbaker | that's the beauty of using model code | 15:29 |
thomasem | I guess it's because of SQLAlchemy | 15:29 |
sulo | i am using orm | 15:29 |
sulo | yeah | 15:29 |
thomasem | sulo: okay, great. Neeevermind! | 15:29 |
jimbaker | model code = SQLAlchemy ORM models for craton | 15:29 |
thomasem | What I get for not pulling it down and checking myself before opening my mouth. | 15:30 |
jimbaker | sulo, i like the idea of driving this from conf vs my idea of just creating user/project | 15:31 |
jimbaker | however, can we can make this a get_object_and_create if necessary pattern? | 15:31 |
jimbaker | basically what happens if it is run multiple times? | 15:32 |
sulo | jimbaker: it should error | 15:32 |
tojuvone | Hi | 15:32 |
fsaad | sulo: can join ? | 15:32 |
jimbaker | yes, that it should do | 15:32 |
sulo | i was going to catch that and throw proper msg back | 15:32 |
jimbaker | but maybe it can report back the API key at that time | 15:32 |
jimbaker | again if you have access to dbsync, you have access to the db. so might as well be useful | 15:33 |
sulo | jimbaker: sure, makes sense | 15:33 |
sulo | whats the suggestion though ? not sure i am following | 15:34 |
sulo | fsaad: joining | 15:34 |
jimbaker | sulo, basically report back the same info as if created | 15:34 |
fsaad | thanks sulo, zz_pwnall1337 is trying to convince me to adopt a pet so be quick | 15:34 |
jimbaker | (it can say however, already created user or project, but here's the relevant info) | 15:35 |
fsaad | please :) | 15:35 |
sulo | jimbaker: oh i see what you are saying ... cool, will do | 15:35 |
jimbaker | cool. basically there are two randomly created pieces of info, project_id and api_key. we don't want the user to have to look into the db with mysql/whatever to look up if they have lost accss | 15:36 |
openstackgerrit | Thomas Maddox proposed openstack/craton master: Move to MySQL 5.7 and SQLAlchemy>=1.1.0 https://review.openstack.org/443186 | 15:45 |
openstackgerrit | Thomas Maddox proposed openstack/craton master: Move to MySQL 5.7 and SQLAlchemy>=1.1.0 https://review.openstack.org/443186 | 15:50 |
thomasem | And it begins. | 15:50 |
sulo | thomasem: for sqlalchemy verison .. you updatd global reqs ? | 15:51 |
thomasem | sulo: global as in top-level requirements.txt? | 15:51 |
thomasem | for Craton... or does that have some additional implication? | 15:51 |
sulo | thomasem: no i mean https://github.com/openstack/requirements/blob/master/global-requirements.txt#L276 | 15:52 |
thomasem | sulo: Oh, no. Did we want to? I thought the update to SQLAlchemy >= 1.1.0 was only for our project? | 15:53 |
sulo | otherewise that will get overwritten next update no ? | 15:53 |
thomasem | I don't know? Will it? Are projects not able to have their own overrides? | 15:53 |
sulo | or does that not happen for < | 15:53 |
sulo | i dunno ... i am curious how it worked | 15:54 |
thomasem | I have no idea. I'm not all that familiar with how that works? | 15:54 |
sulo | i thought the req's check would fail | 15:54 |
sulo | sigmavirus: ^ | 15:54 |
thomasem | I would hope we could override. | 15:54 |
sulo | yeah, same | 15:54 |
sigmavirus | sulo: they should fail | 15:55 |
thomasem | So, what's the proper way to do this in OpenStack land? | 15:55 |
sigmavirus | we opted into g-r automated updates | 15:55 |
sigmavirus | thomasem: propose an update to g-r directly | 15:55 |
sulo | yeah thats what i though | 15:55 |
thomasem | Ouch... I hope that doesn't cause a bunch of grief. | 15:56 |
thomasem | Lemme go do that. | 15:56 |
thomasem | Thanks for calling that out | 15:56 |
thomasem | So, then, I'd submit that review, and say this Depends-On that | 15:57 |
thomasem | In my Craton commit | 15:57 |
thomasem | ? | 15:57 |
thomasem | I was really hoping to not impact a bunch of other projects, was my original intent. | 15:58 |
sigmavirus | thomasem: so bumping minimum versions isn't a big deal | 16:04 |
sigmavirus | that said, the other projects have these changes auto-syncrhonized for them (as do we) | 16:04 |
sigmavirus | so it's fine | 16:04 |
sigmavirus | being part of openstack means being a noisy neighbor | 16:05 |
thomasem | Lol, I see | 16:05 |
thomasem | Okay, thanks sigmavirus sulo :) | 16:05 |
jimbaker | i don't think anyone wants to stick on an old sqlalchemy either. such pinning should at best be temporary, during a specific dev cycle | 16:08 |
jimbaker | otherwise, tech debt of the worst kind | 16:08 |
sigmavirus | jimbaker: it's not pinning | 16:10 |
sigmavirus | it's minimum supported versiono | 16:10 |
sigmavirus | openstack doesn't pin or cap | 16:10 |
sigmavirus | and they tend to bump minimums at the start of each cycle as appropriate and throughout the cycle for projects with hard dependencies on changes in newer versinos | 16:11 |
sigmavirus | thomasem: I have to add a dependency to global-requirements anyway | 16:14 |
sigmavirus | want me to send teh patch to bump the sqla version? | 16:14 |
sigmavirus | Might also be worth checking if anyone else is bumping that before you do it =) | 16:14 |
sigmavirus | Oh sqla *is* capped | 16:24 |
sigmavirus | weird | 16:24 |
sigmavirus | https://review.openstack.org/423192 | 16:24 |
sigmavirus | thomasem: ^ | 16:25 |
jimbaker | interesting commentary, not to mention the keystone specific usage | 16:27 |
*** jovon has joined #craton | 16:27 | |
thomasem | sigmavirus: If you're already under the hood, I would appreciate it! | 16:30 |
thomasem | sigmavirus: ohhh gotcha lookie there | 16:30 |
jimbaker | the other thing is requesting storyboard project creation for craton. ideally we can get this in motion so we can start using next week | 16:32 |
openstackgerrit | Thomas Maddox proposed openstack/craton master: Move to MySQL 5.7 and SQLAlchemy>=1.1.0 https://review.openstack.org/443186 | 16:33 |
thomasem | +1 | 16:33 |
sigmavirus | jimbaker: have you talked to diablo_rojo about timelines for migration? | 16:35 |
sigmavirus | jimbaker: diablo_rojo hangs out in #openstack-dev so might be best to talk to them there about migration details? | 16:36 |
sigmavirus | alternatively #openstack-infra probably | 16:36 |
thomasem | fsaad: you making it to sync? | 16:46 |
fsaad | thomasem: can't, please go on | 16:46 |
fsaad | I did have a meeting a bit ago with sulo and zz_pwnall1337 on which we agreed pwnall has and will push the osic inventory and facts to sulo's demo box before next demo, so things looking good. | 16:49 |
openstackgerrit | Ian Cordasco proposed openstack/python-cratonclient master: Add Betamax for testing https://review.openstack.org/442165 | 16:53 |
*** VW_ has joined #craton | 16:56 | |
*** VW_ has quit IRC | 16:56 | |
*** VW_ has joined #craton | 16:57 | |
*** VW has quit IRC | 16:57 | |
thomasem | fsaad: Sounds good! Thanks for the update. | 17:01 |
fsaad | o/ | 17:05 |
thomasem | https://review.openstack.org/443240 | 17:17 |
thomasem | opt-out of synchronization ^^ | 17:17 |
sulo | jimbaker: thomasem: fsaad: sorry i had to miss the meeting. had to run out to get something and got stuck in traffic | 17:18 |
thomasem | sulo: no worries! | 17:19 |
thomasem | We know you wouldn't miss seeing our glowing faces unless you had something come up. | 17:19 |
fsaad | no prob sulo, I'll cancel the remaining ones, if we have a need to do syncs we can do adhoc | 17:20 |
thomasem | +1 | 17:20 |
sulo | thomasem: heh fsaad: +1 | 17:20 |
openstackgerrit | Thomas Maddox proposed openstack/craton master: Move to MySQL 5.7 and SQLAlchemy>=1.1.0 https://review.openstack.org/443186 | 17:21 |
jimbaker | tojuvone, thanks for the patch, just need a minor change done for https://review.openstack.org/#/c/443129/ and we get in | 17:22 |
tojuvone | jimbaker, Thanks, I'll update | 17:24 |
tojuvone | jimbaker, There would be plenty | 17:25 |
tojuvone | jimbaker, sorry, I was to say about proxy stuff in different places, but that is irrelevant for most | 17:26 |
openstackgerrit | Tomi Juvonen proposed openstack/craton master: docker install documentation not consistent https://review.openstack.org/443129 | 17:30 |
* sigmavirus wishes we were using py.test | 17:59 | |
sigmavirus | --pdb makes everything better | 17:59 |
*** VW_ has quit IRC | 18:03 | |
*** VW has joined #craton | 18:05 | |
*** VW has quit IRC | 18:06 | |
*** VW_ has joined #craton | 18:06 | |
jimbaker | sigmavirus, what do we need to move to pytest? i'm not a regular user of pdb, but i can see it be highly helpful in this case | 18:17 |
openstackgerrit | Merged openstack/craton master: docker install documentation not consistent https://review.openstack.org/443129 | 18:19 |
sigmavirus | jimbaker: so generally trying to pry into running code in tests is a pain with testtools/testr | 18:20 |
sigmavirus | Also they capture all stdout/stderr printing and lose them typically | 18:20 |
sigmavirus | so | 18:20 |
sigmavirus | Print debugging doesn't work | 18:20 |
sigmavirus | log debugging could work but that's not as useful ime | 18:20 |
sigmavirus | So, I'm going through the pains of making pdb work | 18:20 |
sigmavirus | It's not a big deal | 18:20 |
sigmavirus | And switching to pytest wouldn't be a terrible amount of work | 18:21 |
sigmavirus | But it's a distraction | 18:21 |
sigmavirus | I just yearn for testing tools designed in the last 10 years that focus on usability rather than ... Idk what | 18:21 |
sigmavirus | even nosetests provides similar funcationality | 18:21 |
sigmavirus | Even if our craton tox.ini doesn't let use use it directly | 18:21 |
*** Syed__ has joined #craton | 18:45 | |
thomasem | Bah, change to MySQL 5.7 + SQLAlchemy 1.1.6 breaks variables query AND... seeing empty result with SELECT * FROM variables WHERE value_ IN ("juno", "swift"); after upgrade. :\ | 18:59 |
thomasem | Isolating which part breaks it | 18:59 |
thomasem | I'm betting it's the switch to JSON type, because it works when I wrap the values in single quotes. | 18:59 |
sigmavirus | thomasem: interesting | 19:02 |
thomasem | Fun fun | 19:04 |
thomasem | Yep. That's it. JSON column is what breaks it. | 19:06 |
thomasem | SQLAlchmy 1.1.6 + MySQL 5.7 works fine as long as that column winds up being TEXT instead of JSON type. | 19:07 |
sigmavirus | huh | 19:08 |
jimbaker | sigmavirus, agreed about print debugging being less useful with current setup | 19:09 |
jimbaker | thomasem, is this a problem with the use of sqlalchemy_utils? | 19:10 |
thomasem | jimbaker: yeah, it's the way it papered over the lack of JSON support in MySQL | 19:10 |
jimbaker | yeah, sort of what i expected | 19:10 |
jimbaker | so maybe we can propose a patch for them | 19:10 |
thomasem | by storing literal JSON strings. | 19:10 |
jimbaker | it's a pretty small wrapper iirc | 19:11 |
jimbaker | in SA utils | 19:11 |
thomasem | To be friendly? If we're switching to proper JSON columns, why not fix the codebase to handle those values properly? | 19:11 |
thomasem | s/the codebase/craton/ | 19:11 |
thomasem | Assuming there is a 'proper' way, of course. :D | 19:12 |
jimbaker | https://github.com/kvesteri/sqlalchemy-utils/blob/master/sqlalchemy_utils/types/json.py | 19:12 |
thomasem | And it's not just flat-out bugged. | 19:12 |
jimbaker | i think there's a proper way. the json support in sqlalchemy_utils is postgres specific | 19:13 |
jimbaker | i don't see any reason why we cannot just use our own type for now | 19:13 |
thomasem | Well, the problem is partially in the DB. So, the DB tries to be friendly, but in doing so causes us to have a problem when we go to the "correct" way. | 19:13 |
jimbaker | and see if we can get that package to generalize | 19:13 |
jimbaker | not certain i follow | 19:13 |
thomasem | It's storing json.dumps("juno") and json.dumps(True), where you'll see a value_ like '"juno"' and 'true' stored. | 19:14 |
thomasem | Strictly speaking, then, you should have to query for that value like SELECT * FROM variables WHERE value_='"juno"'; | 19:14 |
thomasem | And SELECT * FROM variables WHERE value_='true'; | 19:15 |
thomasem | But, it tries to be friendly and interprets "juno" as '"juno"'. | 19:15 |
jimbaker | yeah. no friendliness please | 19:15 |
thomasem | For TEXT columns | 19:15 |
jimbaker | so we want pure JSON columns | 19:15 |
thomasem | Yeah, it's not always a good idea to be helpful. Winds up with bugs. :P | 19:15 |
thomasem | Yes. | 19:15 |
thomasem | Which means we need to go a stricter route | 19:15 |
thomasem | where you query for '"juno"' | 19:16 |
thomasem | so, you query for json.dumps(value_) | 19:16 |
thomasem | essentially | 19:16 |
thomasem | instead of mixing JSON and strings and having the DB try to be helpful | 19:16 |
thomasem | Using strings will only work for if you're querying for a single value. If you're trying to do an IN (...), it won't work consistently unless you're strict about the values in the list being actual JSON. | 19:17 |
thomasem | Anyway, I hope I explained that well enough. Happy to showcase in Vidyo, too. | 19:17 |
jimbaker | makes sense to me | 19:18 |
thomasem | Okay, good. | 19:19 |
thomasem | So, now I need to figure out how to make it work consistently! | 19:19 |
thomasem | Want to maintain the status quo before adding things on top. | 19:19 |
jimbaker | thomasem, and again, we will be ensuring that JSON types are stored as JSON columns | 19:21 |
jimbaker | so this will stack on my alembic change as well | 19:21 |
jimbaker | so instead of sqlalchemy_utils.types.json.JSONType, we use a real JSON type for that column | 19:23 |
thomasem | jimbaker: yep, that's in my change on top of yours. | 19:23 |
thomasem | Now, the question is, do we want to do that for all occurrences here, or just variables? | 19:23 |
thomasem | I guess one will go away with your alembic change | 19:23 |
thomasem | ("roles") | 19:23 |
thomasem | Assuming that's still in the change | 19:24 |
jimbaker | yeah, roles is gone | 19:24 |
jimbaker | this is exactly why YAGNI needs to apply here :) | 19:24 |
thomasem | vlans is the other one | 19:24 |
sulo | uff .. pymysql raises IntegrityError for duplicate entires | 19:24 |
jimbaker | and that can stay for now | 19:24 |
thomasem | for network_devices | 19:24 |
thomasem | jimbaker: do you mean that can stay sqlalchemy_utils.types.json.JSONType? | 19:25 |
jimbaker | sulo, does pymysql give you the cause? | 19:25 |
jimbaker | no, json means json :) | 19:25 |
jimbaker | no more text | 19:25 |
thomasem | Cool. Agreed. I'll address both, then. | 19:25 |
thomasem | brb | 19:28 |
thomasem | back | 19:41 |
*** VW has joined #craton | 19:57 | |
*** VW has quit IRC | 19:59 | |
*** VW has joined #craton | 19:59 | |
*** VW_ has quit IRC | 20:01 | |
*** VW has quit IRC | 20:03 | |
*** VW has joined #craton | 20:04 | |
*** VW has quit IRC | 20:06 | |
*** VW has joined #craton | 20:06 | |
openstackgerrit | Ian Cordasco proposed openstack/python-cratonclient master: Add Betamax for testing https://review.openstack.org/442165 | 20:10 |
thomasem | saddyface /craton/lib/python3.5/site-packages/pymysql/cursors.py:166: Warning: (1235, "This version of MySQL doesn't yet support 'comparison of JSON in the IN operator'") | 20:50 |
openstackgerrit | Ian Cordasco proposed openstack/python-cratonclient master: Add Betamax for testing https://review.openstack.org/442165 | 20:55 |
jimbaker | thomasem, oh | 21:04 |
*** VW has quit IRC | 21:08 | |
jimbaker | thomasem, and you are running the latest release of PyMySQL right? do you have the full traceback? | 21:13 |
*** VW has joined #craton | 21:22 | |
thomasem | jimbaker: I think it's latest, I'll check. But, that error comes from MySQL 5.7 itself: | Warning | 1235 | This version of MySQL doesn't yet support 'comparison of JSON in the IN operator' | | 21:43 |
thomasem | when I do SHOW WARNINGS | 21:43 |
thomasem | s/error/warning/ | 21:43 |
thomasem | PyMySQL==0.7.10 | 21:44 |
thomasem | that's latest | 21:44 |
thomasem | jimbaker: works fine when I switch to using OR, even though that's a performance hit (saddyface x2). | 21:54 |
thomasem | jimbaker: I rebased and since your change is the parent of mine, I suspect it'll push a rebase up to your change. You cool w/ that? | 22:00 |
jimbaker | thomasem, absolutely fine with that | 22:08 |
jimbaker | and that makes sense, i was wondering where in PyMySQL it was generating that error | 22:09 |
jimbaker | re performance hit - let's focus on functionality first. i think we can figure out any necessary optimizations | 22:09 |
jimbaker | at the right time | 22:09 |
openstackgerrit | Thomas Maddox proposed openstack/craton master: Move to MySQL 5.7 and SQLAlchemy>=1.1.0 https://review.openstack.org/443186 | 22:12 |
openstackgerrit | Thomas Maddox proposed openstack/craton master: WIP: Variable search for resources now uses resolved variables. https://review.openstack.org/440929 | 22:12 |
*** VW has quit IRC | 22:35 | |
*** jovon has quit IRC | 22:42 | |
openstackgerrit | Thomas Maddox proposed openstack/craton master: Move to MySQL 5.7 and SQLAlchemy>=1.1.0 https://review.openstack.org/443186 | 22:50 |
thomasem | Alright, I hear the dinner bell. Have a lovely evening, everyone! | 22:50 |
thomasem | jimbaker: Agreed. I don't want to hamstring us over unknown unknowns. :) | 22:51 |
*** david-lyle has quit IRC | 23:56 |
Generated by irclog2html.py 2.14.0 by Marius Gedminas - find it at mg.pov.lt!