| === Edwin-lunch is now known as EdwinGrubbs | ||
| mwhudson | thumper: i'd ask you to review a branch if you could see it :-) | 03:29 |
|---|---|---|
| thumper | mwhudson: if it is small you could pastebin :) | 03:29 |
| mwhudson | thumper: http://pastebin.ubuntu.com/334267/ | 03:31 |
| mwhudson | thumper: its the next layer of the bzr-svn onion | 03:31 |
| === stub1 is now known as stub | ||
| thumper | mwhudson: I don't suppose you feel like killing updateFromData ? | 03:33 |
| mwhudson | thumper: i could give it a go i guess | 03:34 |
| thumper | config.codeimport.default_interval_subversion, ? | 03:34 |
| thumper | sorry | 03:34 |
| thumper | I missread | 03:34 |
| thumper | nn | 03:34 |
| thumper | nm | 03:34 |
| thumper | damn friday fingers | 03:34 |
| thumper | mwhudson: line 171 of diff | 03:35 |
| thumper | mwhudson: what about using the with statement ? | 03:35 |
| thumper | mwhudson: won't it close files on context exit? | 03:36 |
| thumper | mwhudson: the for loop at 181... what is the purpose? | 03:37 |
| thumper | shouldn't it error if it can't connect at all in the setup? | 03:41 |
| mwhudson | thumper: for the former, wow yes, python 2.5 :) | 03:55 |
| mwhudson | thumper: it takes a little while for the svnserve process to start accepting connections, that's what the for loop is for | 03:55 |
| thumper | mwhudson: my suggestion is that if we don't get acceptance perhaps the setup should fail | 03:55 |
| mwhudson | thumper: no, it usually connects the second time around the loop | 03:56 |
| mwhudson | oh i see what you mean now | 03:56 |
| mwhudson | friday brain :) | 03:56 |
| thumper | I know what you mean | 03:56 |
| thumper | mwhudson: other than that it looks good | 04:02 |
| mwhudson | thumper: thanks! | 04:02 |
| thumper | mwhudson: do we have the BZR_SVN enum in prod already? | 04:02 |
| thumper | mwhudson: it would be interesting to see what happens on vanilla production branch looking at a bzr_svn import | 04:02 |
| mwhudson | thumper: yeah, it's been around for ages | 04:02 |
| thumper | mwhudson: as long as it doesn't die in a heap, we should be ok | 04:03 |
| mwhudson | thumper: yeah, i'll see if i can do that locally | 04:03 |
| mwhudson | thumper: i know +code-import-list will explode | 04:03 |
| thumper | mwhudson: ah, why? | 04:03 |
| thumper | I don't remember that view | 04:03 |
| mwhudson | thumper: getImportDetailsForDisplay | 04:04 |
| mwhudson | (another reason we should move to a single url field, i think...) | 04:05 |
| * thumper nods | 04:15 | |
| thumper | mwhudson: movie and pizza time, I'll check back in later :) | 04:19 |
| mwhudson | thumper: certainly, don't watch the cricket | 04:19 |
| === adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === matsubara-afk is now known as matsubara | ||
| henninge | bigjools: care to review? ;) http://paste.ubuntu.com/334439/ | 11:41 |
| bigjools | henninge: +1 | 11:42 |
| henninge | bigjools: as in r=bigjools? ;-) | 11:42 |
| bigjools | henninge: r=bigjools :) | 11:43 |
| henninge | bigjools: I made it a formal MP to be able to properly request an r-c. Please add your vote there, too. Thanks ;) | 11:54 |
| henninge | https://code.edge.launchpad.net/~henninge/launchpad/access-to-translationgroup-table/+merge/15643 | 11:54 |
| bigjools | sure | 11:54 |
| bigjools | henninge: thinking about it, I think I'd be happier with a test | 11:56 |
| * henninge was just thinking about that on the loo ... | 11:57 | |
| === adeuring1 is now known as adeuring | ||
| bigjools | TMI | 11:58 |
| henninge | bigjools: how would I test that ... | 11:58 |
| bigjools | henninge: the best test is to upload a package with an attached translation that would need to use the code you changed | 11:59 |
| henninge | bigjools: so this happens whenever I store an upload in the librarian? | 11:59 |
| henninge | that should not be too hard to simulate | 12:00 |
| bigjools | henninge: soyuz calls sourcepackagerelease.attachTranslationFiles() | 12:01 |
| bigjools | that's all I know :) | 12:01 |
| bigjools | let me see if there's some existing tests you can copy | 12:01 |
| henninge | the traceback goes through realiseUpload - publish - publish_ROSETTA_TRANSLATIONS | 12:02 |
| henninge | oh, nm, missed the next line | 12:02 |
| bigjools | hmmm it appears there are no tests for uploading translations at all | 12:03 |
| bigjools | which is not surprising since this bug would have been caught | 12:04 |
| bigjools | henninge: we need a package with translations attached in lib/lp/archiveuploader/tests/data/suite | 12:05 |
| bigjools | then add a test in lib/lp/archiveuploader/tests/test_uploadprocessor.py | 12:05 |
| bigjools | I am happy to help | 12:05 |
| bigjools | since it's probably making your eyes glaze already | 12:05 |
| henninge | bigjools: ok, I'll look at it and come back to pester you ... ;) | 12:05 |
| BjornT_ | henninge, bigjools: uploading a package with translation is good as an integration test, but it's also good to have move specific tests that only call the method in question. that test can be run as a number of different db users. | 12:12 |
| bigjools | BjornT_: well right now there are *no* tests that upload packages with translations | 12:13 |
| bigjools | I find it hard to see why the translations team should write a test that calls their code using a soyuz db user | 12:13 |
| BjornT_ | bigjools: right, and i'm not saying that you shouldn't add one :) i just want to avoid you adding too many, so that you don't use integration tests to test edge cases | 12:14 |
| henninge | we are all one team ... ;) | 12:14 |
| bigjools | yes, I would not suggest adding more than one :) | 12:14 |
| BjornT_ | bigjools: they should write such tests, if their code is executed in a soyuz db user context | 12:14 |
| BjornT_ | bigjools: that's a good way of avoiding errors like these | 12:15 |
| bigjools | BjornT_: they should, and the test should run in the Soyuz area | 12:15 |
| BjornT_ | bigjools: what do you mean by 'in the soyuz area'? | 12:16 |
| bigjools | i.e. I don;t think the translations tests should know about soyuz specifics | 12:16 |
| bigjools | in our test directory | 12:16 |
| * henninge is thinking | 12:16 | |
| BjornT_ | bigjools: that's the point, they only have to know about the db user, nothing more | 12:16 |
| bigjools | so once we have a test that uploads a translations package, there can be unit tests in soyuz module that call the SPR methods | 12:17 |
| BjornT_ | bigjools: it depends a bit on which method is tested, of course | 12:17 |
| bigjools | yeah I don;t really like that they need to know about it. Am I being too sensitive about that? | 12:17 |
| henninge | bigjools: is it not the same the other way round? | 12:17 |
| henninge | I mean, if we add a translations upload test to the soyuz test directory? | 12:18 |
| BjornT_ | bigjools: but for example, look at lp/bugs/doc/bug-set-status.txt. where should that live? | 12:18 |
| bigjools | soyuz uses translations, translations doesn't use soyuz | 12:18 |
| BjornT_ | bigjools: that's one of tests to make sure that closing bugs via changelogs works | 12:18 |
| bigjools | BjornT_: yeah, we have a similar one the in the archiveuploader module | 12:19 |
| BjornT_ | bigjools: if it's very similar, it's probably redundant | 12:19 |
| bigjools | lib/lp/archiveuploader/tests/nascentupload-closing-bugs.txt | 12:19 |
| bigjools | and lib/lp/soyuz/doc/closing-bugs-from-changelogs.txt | 12:20 |
| bigjools | BjornT_: if you think it's ok for translations to know about soyuz db users then I am fine with it | 12:21 |
| bigjools | I just wanted to raise the question | 12:21 |
| BjornT_ | bigjools: i think it's fine for now. we should find a better way of structuring it, so that it's easier to find all the tests related to a given feature (it's a bit tricky, since one test can belong to multiple features). at the moment either translations have to know a bit about soyuz, or soyuz have to know a bit about translations. which is better? | 12:23 |
| bigjools | soyuz already knows a lot about translations :) | 12:24 |
| bigjools | it has to deal with the tarballs attached to source packages | 12:24 |
| BjornT_ | bigjools: maybe it knows too much :) | 12:24 |
| bigjools | heh | 12:25 |
| BjornT_ | bigjools: i just read henninge's mail. that tells me that there probably are tests already for setStatus and addOrUpdateEntry, and those tests should be run using the soyuz db user (as well as the db users it currently runs with) | 12:28 |
| bigjools | right | 12:28 |
| BjornT_ | bigjools: it might make sense to have a tests harness in soyuz land, but the tests should be re-used | 12:28 |
| henninge | bigjools, BjornT_: also, that sounds to me like a much quicker way to get a valid test set up for this branch ... ;-) | 12:28 |
| bigjools | well I still want a full translations package upload test | 12:28 |
| BjornT_ | henninge: what bigjools said | 12:29 |
| bigjools | henninge: yep | 12:29 |
| bigjools | I'd be ok with him landing this with that shorter test, as long as a bug is file to write the bigger test, what do you think BjornT_? | 12:30 |
| henninge | bigjools: was about to ask that, too. | 12:30 |
| BjornT_ | bigjools: which is the shorter test? | 12:30 |
| bigjools | BjornT_: re-using existing tests with a new db user | 12:31 |
| BjornT_ | bigjools: right. i think that's the easiest way of trying to prevent errors like this to happen in the future. i'd be happy with those tests for the first landing | 12:32 |
| bigjools | great | 12:32 |
| bigjools | makes your life easier for the moment henninge :) | 12:32 |
| henninge | cool | 12:32 |
| * henninge will do that right after lunch. | 12:33 | |
| === henninge is now known as henninge-lunch | ||
| bigjools | mmm food | 12:33 |
| === mrevell is now known as mrevell-lunch | ||
| noodles775 | Hi adeuring, got time for this one? | 13:26 |
| noodles775 | https://code.edge.launchpad.net/~michael.nelson/launchpad/487009-db-extract-get-build-records/+merge/15649 | 13:26 |
| adeuring | noodles775: sure | 13:29 |
| noodles775 | Thanks! | 13:32 |
| === salgado-afk is now known as salgado | ||
| === henninge-lunch is now known as henninge | ||
| === mrevell-lunch is now known as mrevell | ||
| adeuring | noodles775: r=me | 15:07 |
| noodles775 | Thanks adeuring. | 15:08 |
| salgado | henninge, that db permission fix is not release-critical, since stub's already done it on the production DB, right? | 15:15 |
| === salgado is now known as salgado-lunch | ||
| sinzui | barry: Your mailing list branch is approved. I had two points you may want to address before landing it | 15:30 |
| barry | sinzui: awesome, thanks | 15:30 |
| henninge | salgado-lunch: I am not sure how persistent stubs change is and what happens when "something" is restarted or rebuilt etc. | 15:33 |
| henninge | something = appserver or dbserver or stub's car ... ;) | 15:34 |
| === salgado-lunch is now known as salgado | ||
| === matsubara is now known as matsubara-lunch | ||
| salgado | henninge, indeed; I was thinking of a schema change, and if that was the case it would only be possible to revert it manually. but since this is actually a permission change, it might be possible to revert it accidentally if someone applies security.cfg to the DB | 15:46 |
| === adeuring1 is now known as adeuring | ||
| === beuno is now known as beuno-lunch | ||
| james_w | anybody fancy reviewing a lazr.restfulclient branch? https://code.launchpad.net/~james-w/lazr.restfulclient/fix-caching/+merge/15635 | 16:20 |
| james_w | tiny diff, ugly bug | 16:20 |
| === deryck is now known as deryck[lunch] | ||
| jpds | hello. | 16:33 |
| noodles775 | james_w: adeuring is OCR - so worth pinging him :) | 16:34 |
| adeuring | james_w: sure, I'll look | 16:34 |
| === matsubara-lunch is now known as matsubara | ||
| adeuring | james_w: this looks like a really good bug fix! Just two formal nitpicks: (1) Could you move the last parameter in the changed call on a new line? we try to keep the length <= 78 characters. (2) I think most of your commit message is worth to appear as a comment for the changed method call so that readers know why http_method.upper() makes sense here? | 16:45 |
| james_w | makes sense | 16:45 |
| adeuring | james_w: thanks! r=me | 16:46 |
| james_w | adeuring: new revision pushed, could you land it for me, I don't have the rights? | 16:52 |
| adeuring | james_w: sure, will do. | 16:52 |
| james_w | thanks | 16:52 |
| bac | hey jpds -- i was going to chat with you about the review but then just submitted it anyway | 17:00 |
| henninge | bigjools: I just pushed the branch. | 17:02 |
| henninge | bigjools: After a not-so-successful high-level approach (calling addOrUpdateEntryFromTarball), I went for low-level testing on canSetStatus | 17:03 |
| henninge | bigjools: it actually produced more problems with access to pofile and potemplate tables, so I included these, too. | 17:03 |
| bigjools | henninge: can a translations person review it for you? | 17:03 |
| henninge | danilos: ? | 17:03 |
| henninge | danilo__: ? | 17:04 |
| bigjools | CaptainAmerica to the rescue | 17:04 |
| henninge | are you sure he has an alarm on that name? | 17:04 |
| henninge | he's also the only one from my team around. | 17:05 |
| jpds | bac: Is it just the registered by Mark text I have to edit? | 17:05 |
| bac | jpds: yes. it just needs to match the new output | 17:06 |
| bac | jpds: test it with: bin/test -vvt distributionmirror-views.txt | 17:06 |
| === beuno-lunch is now known as beuno | ||
| sinzui | beuno: I am still trying to reconcile how to handle official verses unofficial packages for projects. look at https://edge.launchpad.net/bzr | 17:22 |
| sinzui | ^ beuno, the "Packages in distributions" portlet does not have a version line for the unofficial packages. Should I hide them everywhere or do it make it clear that they are only available via PPA | 17:24 |
| sinzui | or some other archive | 17:24 |
| === deryck[lunch] is now known as deryck | ||
| sinzui | We really cannot hide them if we do not know the version, because we can only know the Ubuntu versions--we do not track versions in any other distro | 17:27 |
| beuno | sinzui, give me 30 and we'll think about it together | 17:27 |
| jpds | bac: OK, will work on it when I can time. | 17:33 |
| sinzui | beuno: I have a lunch appointment. I will be back in a hour. | 17:41 |
| === gary_poster_ is now known as gary_poster | ||
| bac | jpds: when you make the changes let me know and i'll update the review and land it for you, if PQM has opened by then. | 19:21 |
| jpds | bac: Done. | 20:02 |
| bac | jpds: it still doesn't work, though. You left out "Linux" in the mirror title | 20:05 |
| bac | jpds: run: /bin/test -vvm lp.registry -t distributionmirror-views.txt | 20:06 |
| bac | jpds: oops | 20:06 |
| bac | bin/test -vvm lp.registry -t distributionmirror-views.txt | 20:06 |
| bac | (no leading slash, of course) | 20:06 |
| * jpds ponders, the only thing I did with "Linux" was remove it from "Official Ubuntu Linux Mirror registered by ...". | 20:08 | |
| jpds | And it shouldn't be on mirror pages: https://edge.launchpad.net/ubuntu/+mirror/archive.ubuntu.com | 20:09 |
| bac | jpds: you did remove the word "Linux" from the expected test output | 20:11 |
| bac | "Ubuntu Linux" is the context/title | 20:12 |
| bac | jpds: so stuff "Linux" back in there and you will be good | 20:12 |
| jpds | bac: But if Linux isn't displayed on the live pages... why is it in the tests? | 20:17 |
| * bac looks | 20:17 | |
| bac | jpds: if you look here you'll see that the text is the same as it appears in the test: https://launchpad.dev/ubuntu/+mirror/archive-mirror2 | 20:22 |
| bac | that's because the distro title is "Ubuntu Linux". the name property is simply "Ubuntu" | 20:23 |
| jpds | Oh, right. | 20:24 |
| === matsubara is now known as matsubara-afk | ||
| EdwinGrubbs | sinzui: are you still looking for a reviewer, or is the irc topic just stale? | 20:47 |
| jpds | bac: Well I've made the change as you've said, can't get ec2test to work nicely though. | 20:47 |
| sinzui | EdwinGrubbs: That is stale | 20:47 |
| === sinzui changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| bac | jpds: if you've never used ec2 i wouldn't bother now. there is quite a bit of setup involved. | 20:47 |
| bac | jpds: did you run the simple local test i listed? | 20:48 |
| jpds | bac: No, I'll work on it later. | 20:48 |
| bac | jpds: it's a one-liner. should only take a few seconds | 20:49 |
| bac | jpds: nm, i grabbed your new branch and the test passes | 20:53 |
| bac | jpds: one last thing. if you go to https://code.edge.launchpad.net/~jpds/launchpad/fix_196703/+merge/15644 and set the commit message i'll be able to land it for you through ec2. just include the text of the message, not the review name or any of that stuff. | 20:54 |
| jpds | bac: Sorry, was having food, and done. | 21:55 |
| === salgado is now known as salgado-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!