| cjwatson | Could I have a review of https://code.launchpad.net/~cjwatson/lp-signing/+git/lp-signing/+merge/382431 (Run migrations in charm), please? | 09:56 |
|---|---|---|
| tomwardill | cjwatson: you have migrations happening before service.configured, is that the right order? | 10:00 |
| cjwatson | tomwardill: I think so - doesn't seem like a good idea to claim that the app is up before its DB schema is up to date | 10:01 |
| tomwardill | fair | 10:01 |
| tomwardill | have a +1 :) | 10:01 |
| cjwatson | (In fact, gunicorn starts on service.configured, so we definitely need to avoid doing that early.) | 10:03 |
| cjwatson | There is a certain amount of puzzle-box programming here to figure out exactly how to avoid tickling the OLS layers in the wrong way. | 10:03 |
| cjwatson | Thanks | 10:03 |
| * tomwardill fetches more coffee | 10:05 | |
| tomwardill | do we want users to be able to create credenti'als with the same url and username? | 10:58 |
| tomwardill | I'm leaning towards we should check for that | 10:58 |
| cjwatson | It seems likely to be confusing | 11:00 |
| * tomwardill adds the check | 11:00 | |
| tomwardill | but first, lunch | 11:00 |
| cjwatson | Ah that's right, didn't add a DB constraint because we weren't really sure enough of what form creds would take I think | 11:01 |
| tomwardill | Yeah, still not sure. The first set is obviously going to be username and password, but tokens are a possibility later. | 11:44 |
| tomwardill | Doing code checks for it makes the most sense, as we’ll need to change the push code to handle anything other than None and username/password | 11:44 |
| cjwatson | Not to mention the UI. | 11:46 |
| tomwardill | Oh yes, that too. | 11:46 |
| cjwatson | https://code.launchpad.net/~cjwatson/lp-signing/+git/lp-signing/+merge/382581 brown-paper-bag fix to DB setup, please review | 12:23 |
| tomwardill | cjwatson: +1 | 12:34 |
| tomwardill | cjwatson: do we have any precedence for a 'createOrExisting' type method? | 14:30 |
| tomwardill | I'm thinking Registry Credentials could use that | 14:31 |
| tomwardill | maybe get_or_create is a better description | 14:31 |
| cjwatson | tomwardill: Yeah, there are various | 14:37 |
| cjwatson | tomwardill: Several getOrCreateFoo | 14:37 |
| cjwatson | tomwardill: ensureFoo is also an option | 14:37 |
| tomwardill | ah, excellent | 14:38 |
| tomwardill | I shall try some variant of that | 14:38 |
| * tomwardill blergs at the web service | 15:42 | |
| tomwardill | getting a 401 when I try to call my new method, when I am definitely the owner | 15:44 |
| pappacena | Is there anything on the response's body? | 15:45 |
| tomwardill | "(<lp.oci.model.ocirecipe.OCIRecipe object at 0x7f3fa1f8c250>, 'createPushRule', 'launchpad.Edit')" | 15:45 |
| pappacena | strange... did you try `user.isOwner(the_oci_recipe_object)`, just to make sure? | 15:47 |
| cjwatson | I usually find that some bit of ownership is not what I think it is ... | 15:47 |
| tomwardill | is there a way to debug this? | 15:47 |
| pappacena | On security.py, put a breaking point at EditOCIRecipe.checkAuthenticated | 15:48 |
| cjwatson | Yep | 15:49 |
| cjwatson | Alternatively, breakpoint on zope.publisher.publish.mapply is a bigger hammer if you need it | 15:49 |
| cjwatson | That should be shortly before the traversed-to method is entered | 15:49 |
| tomwardill | oh, maybe I'm doing a silly | 15:49 |
| cjwatson | (But EditOCIRecipe.checkAuthenticated is an easier place to start, probably) | 15:50 |
| tomwardill | narrator: he was doing a silly. | 15:50 |
| tomwardill | (forgot the owner= on factory.makeOCIRecipe) | 15:50 |
| cjwatson | 16:47 <cjwatson> I usually find that some bit of ownership is not what I think it is ... | 15:50 |
| cjwatson | :-) | 15:50 |
| pappacena | hahaha! Been there, done that... | 15:51 |
| tomwardill | NoCanonicalUrl: No url for <lp.oci.model.ocipushrule.OCIPushRule object at | 15:51 |
| tomwardill | ah, yes | 15:51 |
| tomwardill | that | 15:51 |
| cjwatson | pappacena: Could you have a quick look at https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/382592 ? | 15:51 |
| * pappacena sure! | 15:52 | |
| cjwatson | Should fix an OOPS I hit on dogfood | 15:52 |
| pappacena | Tom, URL tag on zcml... export the new entity on webservice.py... | 15:52 |
| pappacena | Ahm... I didn't realise the signing process uses a different db user. Thanks for the MP, cjwatson | 15:53 |
| cjwatson | Easy to miss | 15:55 |
| cjwatson | pappacena: Did you notice http://lpbuildbot.canonical.com/builders/lp-devel-xenial/builds/1192/steps/shell_9/logs/summary ? | 16:06 |
| pappacena | No... let me check | 16:06 |
| cjwatson | and/or do you need help debugging it? | 16:07 |
| cjwatson | Ugh, passes for me locally | 16:08 |
| pappacena | I'll try to reproduce... but I don't recognize those tests... | 16:08 |
| pappacena | uhm... | 16:09 |
| pappacena | there was a timeout trying to start rabbitmq at the begining... could that be the cause? | 16:09 |
| cjwatson | pappacena: Ah, the patch that triggered this was just adding DB tables, so buildbot is probably unfairly blaming you | 16:10 |
| cjwatson | It's possible | 16:10 |
| pappacena | Let me force build it again | 16:10 |
| cjwatson | I've just retried, sorry for the noise | 16:10 |
| pappacena | No problem :) | 16:10 |
| tomwardill | testtools.matchers._impl.MismatchError: 200 != 404: Object: <lp.oci.model.ocirecipe.OCIRecipe object at 0x7f8c1bebfe10>, name: u'string:distribution-100004' | 16:21 |
| tomwardill | you're not wrong there testtools, you're not wrong | 16:21 |
| cjwatson | That string: is slightly suspicious | 16:22 |
| cjwatson | Feels like a buggy <browser:url/> | 16:22 |
| tomwardill | yeah, that would make sense | 16:22 |
| tomwardill | I'm failing to define the correct url for a pushrule, apparently | 16:23 |
| tomwardill | any ideas why "path_expression="string:+pushrule/${id}" is producing: 'http://api.launchpad.test/devel/~person-name-100000/distribution-100004/+oci/oci-project-name-100017/+recipe/oci-recipe-name-100018/+pushrule/None' | 16:38 |
| tomwardill | the None there isn't exactly right... | 16:38 |
| pappacena | Strange. Is it really commited to database? Did you check if the OCIPushRule object actually has an ID at that point? | 16:41 |
| cjwatson | ids are typically serials which are generated by the DB in the process of adding the row, so you may need to flush after adding the object to the store in order to get the id back | 16:42 |
| cjwatson | (commit would do it too but is overkill) | 16:42 |
| cjwatson | Another small fix found while QAing the signing service: https://code.launchpad.net/~cjwatson/lp-signing/+git/lp-signing/+merge/382598 | 16:50 |
| pappacena | I'll check | 16:50 |
| tomwardill | okay, that fixed my id, now to work out why I'm getting a 404 :) | 16:51 |
| pappacena | New @stepthrough method on OCIRecipeNavigation ? | 16:56 |
| tomwardill | yep, that's the one. Ta pappacena :) | 17:02 |
| * pappacena Good! :) | 17:02 | |
| tomwardill | Total: 217 tests, 0 failures, 0 errors, 0 skipped in 2 minutes 55.683 seconds. | 17:14 |
| tomwardill | well, that'll do. Cleaning up the mess I've made along the way is a job for FutureTom | 17:14 |
| * tomwardill -> EOD :) | 17:14 | |
| cjwatson | https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/382606 (Add UI for editing Distribution.oci_project_admin) | 19:52 |
| * pappacena I'll check | 19:59 | |
| cjwatson | I've done a bunch of QA and intend to do a rollout tonight; we need to get it out this week, but rollouts for the next few days will have to be extra-careful to avoid disrupting the Ubuntu release, so it has to be now | 20:02 |
| cjwatson | (Not ideal timing, but there you go) | 20:03 |
| cjwatson | Shout if you know of anything wrong | 20:03 |
| pappacena | Ok... let me know if something doesn't work properly... | 20:03 |
| cjwatson | Actually ... bother, I just realised that the comment about bug contacts in https://bugs.launchpad.net/launchpad/+bug/117752 still stands | 20:07 |
| mup | Bug #117752: Any logged in user can delete any attachments <lp-bugs> <Launchpad itself:In Progress by pappacena> <https://launchpad.net/bugs/117752> | 20:07 |
| cjwatson | Which we discussed briefly in the MP, but I'd missed that comment about retracers deleting attachments | 20:08 |
| pappacena | retracers? | 20:08 |
| cjwatson | Ubuntu has an automatic crash reporting system which in some cases creates bugs with core dumps attached, but those have to be private since they often contain lots of confidential information; there's a service that goes through core dumps attached to bugs in this way and extracts tracebacks from them, then deletes the core dumps, which makes it less of a privacy minefield | 20:10 |
| cjwatson | But the "deletes the core dumps" bit is kind of crucial | 20:10 |
| cjwatson | Sorry, I should have thought of that wrinkle earlier | 20:11 |
| pappacena | Ahh... what permission should we use to allow them to delete too? | 20:11 |
| cjwatson | bug_supervisor, but it involves looking at bug tasks | 20:11 |
| cjwatson | Because that's on the pillar | 20:11 |
| cjwatson | But then we have to take more care about performance because bugs might have lots of bug tasks | 20:12 |
| cjwatson | EditBug._hasAnyRole is similar and would probably do | 20:13 |
| cjwatson | Let me think for a bit about whether we can get away with taking that slow approach | 20:13 |
| pappacena | Ok... | 20:14 |
| cjwatson | pappacena: I think we can. Mind if I take this? We're in a bit of a rush and I think I can do it faster than I can explain it :) | 20:16 |
| pappacena | Sure, no problem! :-) | 20:17 |
| cjwatson | pappacena: https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/382608 | 20:35 |
| * pappacena checking | 20:39 | |
| cjwatson | Thanks, landing | 20:49 |
| cjwatson | pappacena: Before I forget - nice work on the lp-signing integration so far. Once I fixed all the things that were essentially my fault (charm bugs, incorrect firewall rule, missing escaping in openssl's command line), it all seems to work just fine. | 20:54 |
| cjwatson | Though I don't think we should actually attempt to enable it until after the Ubuntu release, even if we have a production environment by then :-) | 20:55 |
| pappacena | Thanks, cjwatson! And actually, thanks for all the reviews and the work on the infrastructure... my part was actually quite small compared to this. | 21:06 |
| pappacena | And I agree to play it safe and wait for Ubuntu release before we enable this in production. | 21:06 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!