| wgrant | wallyworld_: Because the team has access, a grant is not created for the member. | 00:00 |
|---|---|---|
| wgrant | wallyworld_: So they only have access through the team. | 00:00 |
| wgrant | When the team's access is revoked from +sharing, a REmoveGranteeSubscriptionsJob removes the team's subscription. | 00:00 |
| wgrant | But doesn't remove the person's subscription, even though they no longer have access. | 00:00 |
| wallyworld_ | oh, from +sharing. the remove subscription for former members works when using the team page to revoke membership | 00:01 |
| wallyworld_ | so there could be a case to drop the RemoveGranteeSubscriptionsJob and just rely on the RemoveBugSubscriptionsJob | 00:02 |
| wgrant | wallyworld_: I think they should indeed be merged. | 00:03 |
| wgrant | but RemoveBugSubscriptionsJob would need to not require bug_ids. | 00:03 |
| wallyworld_ | probably, it will need to be looked at | 00:03 |
| wallyworld_ | the RemoveGranteeSubscriptionsJob was written first and then several pipes later the RemoveBugSubscriptionsJob came along | 00:04 |
| wgrant | Ah, I see. | 00:05 |
| StevenK | Perhaps we should just use RemoveGranteeSubscriptionsJob and have it for both branches and bugs | 00:06 |
| wallyworld_ | StevenK: that job was written to handle both | 00:07 |
| StevenK | Then why does RemoveBugSubscriptionsJob exist? :-) | 00:08 |
| wallyworld_ | StevenK: the former removes access for an individual subscriber from the specified bug/branches for a specified pillar | 00:08 |
| wallyworld_ | the later removes subscriptions for any artifacts any person can no longer see | 00:09 |
| wallyworld_ | for any pillar | 00:09 |
| wallyworld_ | the former was written early on to work will the +sharing page | 00:09 |
| wallyworld_ | the latter was written to handle team membership revocation and evolved from there | 00:10 |
| wgrant | The rules for both should be the same. | 00:10 |
| wgrant | But one is information_type-wide | 00:10 |
| wgrant | And the other is restricted to a set of bugs. | 00:10 |
| wgrant | Both need to consider teamparticipation. | 00:10 |
| wallyworld_ | one is potentially more efficient because it is more constrained | 00:11 |
| wallyworld_ | so perhaps both are needed, but the remove gants one needs to include team membership | 00:11 |
| wgrant | Why are both needed? | 00:11 |
| wgrant | One has bugtaskflat.information_type = 3 | 00:11 |
| wgrant | The other has bugtaskflat.bug IN (1, 2, 3, 4) | 00:11 |
| wgrant | They should otherwise be identical, AFAICT | 00:11 |
| wallyworld_ | i did say "potentially" - i need to review the code | 00:12 |
| wgrant | Yeah | 00:12 |
| wallyworld_ | the one from the +sharing page is constrained to that pillar so potentially may have less work to do in the db | 00:12 |
| wallyworld_ | bugtaskflat.bug IN (1, 2, 3, 4) will return far less rows that infotype = 3 | 00:13 |
| wallyworld_ | hence the downstream processing may be less | 00:14 |
| wgrant | Er yeah, I mean (bugtaskflat.information_type = 3 and bugtaskflat.product = 1) or (bugtaskflat.bug IN (1, 2, 3, 4)) | 00:14 |
| wgrant | The two types of constraints are required for efficiency. | 00:14 |
| wallyworld_ | still less rows | 00:14 |
| wgrant | But the implementation can otherwise be identical. | 00:14 |
| wgrant | Just one filters by infotype and pillar, the other by a predefined list of bug IDs. | 00:14 |
| wallyworld_ | yes, the implementations can likely be merged now that both have been written | 00:15 |
| wallyworld_ | but the different filtering should remain probably | 00:15 |
| wgrant | Yup | 00:15 |
| wgrant | wallyworld_: btw, bin/celeryd -l DEBUG -Q job --config lp.services.job.celeryconfig | 00:38 |
| wgrant | Handy for testing stuff outside tests. | 00:38 |
| wgrant | Without waiting 50s for the cronjob to start | 00:38 |
| wallyworld_ | cool, i did know that a couple of weeks ago but forgot | 00:39 |
| lifeless | ahahaha http://www.leakedin.com/ | 01:21 |
| lifeless | also leakedin.org :> | 01:25 |
| wallyworld_ | StevenK: what did we decide about have a public A stacked on public B but then making B userdata - A should also become userdata? | 02:00 |
| StevenK | wallyworld_: Hm, I think I've swapped that out. Let me think. | 02:01 |
| wallyworld_ | ok. i have a test which does that and fails | 02:01 |
| wallyworld_ | it used to set private = true and the triggers would update everything | 02:02 |
| wallyworld_ | but setting info type = userdata fails since A remains public | 02:02 |
| StevenK | wallyworld_: Right, so the information_type won't change, but we check if the stacked_on branches information_type is private in places where we were depending on transivitely_private. | 02:05 |
| wallyworld_ | StevenK: sure, we now check for branches "under" the one being changed, but the old transitively private stuff also went "up" the chain | 02:06 |
| StevenK | Yes, we don't do that. | 02:07 |
| wallyworld_ | i think we need to | 02:07 |
| StevenK | Why? | 02:07 |
| wallyworld_ | or else we will have public stacked on user data and will be in the same mess | 02:07 |
| wallyworld_ | as we had before transitively private | 02:07 |
| wgrant | wallyworld_: It's less than practical to do that universally, and the cases where it's useful are less than common. | 02:09 |
| wallyworld_ | it's not impractical if we use triggers | 02:09 |
| wgrant | wallyworld_: If you're changing a stacked-on branch from public to private, you should know you're already in strife. | 02:09 |
| StevenK | Neither wgrant or I want to use triggers. | 02:09 |
| wgrant | wallyworld_: It's still impractical. | 02:09 |
| wallyworld_ | why impractical? | 02:10 |
| wgrant | wallyworld_: It's just that it's so unused nowadays that the impractical cases haven't shown up yet. | 02:10 |
| wallyworld_ | and if we don't fix this, users will shoot themselves in the foot | 02:10 |
| wgrant | wallyworld_: Writing to a few thousand branches because of a write on one branch is silly. | 02:10 |
| wallyworld_ | that's what we do now with the triggers and it works fine | 02:10 |
| wgrant | wallyworld_: Users have shot themselves in the foot in the past due to Launchpad staff misconfiguring branch visibility policies. | 02:10 |
| wgrant | sinzui has since fixed them, and in the new model it's impossible. | 02:11 |
| wallyworld_ | so are you sating allowing a public branch to stack on a private branch is ok? | 02:11 |
| wallyworld_ | because the code as written allows that | 02:11 |
| wgrant | wallyworld_: The normal footgun situation is handled here, as it was before. If you push up a new public branch stacked on a private branch, it will be overridden to private. | 02:11 |
| wgrant | What's not handled is making an existing public stacked-on branch private. | 02:12 |
| wallyworld_ | wgrant: sure, but now the other way arpund | 02:12 |
| wallyworld_ | not | 02:12 |
| wgrant | Now, we should probably warn in that case. | 02:12 |
| wgrant | But it's going to be extremely rare. | 02:12 |
| wgrant | Extremely. | 02:12 |
| wallyworld_ | you can make a stacked on branch private | 02:12 |
| wallyworld_ | the code allows it | 02:12 |
| wgrant | Sure. | 02:12 |
| wallyworld_ | so people will do it | 02:12 |
| wallyworld_ | there's a test which does it | 02:12 |
| wgrant | It's possible to do. | 02:13 |
| wallyworld_ | i think we are leaving a huge hole | 02:13 |
| wgrant | +sharing will say "you have n private branches" | 02:13 |
| wallyworld_ | and don't understand why we are regressing | 02:13 |
| wgrant | The public branch will probably have a warning say "wtf are you crazy" | 02:13 |
| wgrant | But the situation should never occur again. | 02:13 |
| wgrant | Unless someone forgets to set up private branches before they push their code. | 02:13 |
| wallyworld_ | should !+ won't | 02:14 |
| wallyworld_ | it will and can occur via the api i think (need to check) | 02:14 |
| wgrant | I'm not saying that it's impossible to do. | 02:14 |
| wgrant | I'm saying that it is very unlikely to happen. | 02:14 |
| wallyworld_ | the system should not allow people to do this | 02:14 |
| wgrant | Unless someone deliberately does it. | 02:14 |
| wallyworld_ | unlikely != won't | 02:14 |
| wallyworld_ | well they may not realise | 02:14 |
| wallyworld_ | what's to stopme from changing my branch to private not realisng it is stacked on | 02:15 |
| wgrant | Nothing. | 02:15 |
| wgrant | But why would you do that? | 02:15 |
| wallyworld_ | what, change a public branch to private? | 02:15 |
| wgrant | Yes. | 02:15 |
| wallyworld_ | why not? | 02:15 |
| wgrant | Why? | 02:15 |
| wallyworld_ | you may want to do future work in secret | 02:16 |
| wgrant | Anyway. Doing unbounded cascading writes is foolish, and this case is sufficiently harmless and so extraordinarily uncommon that I'm pretty sure it's not worth it. | 02:17 |
| wallyworld_ | it's not unbounded really | 02:17 |
| wgrant | There may be some sense in adding a warning to the stacked branch. | 02:18 |
| wgrant | It is unbounded. | 02:18 |
| wallyworld_ | not in practice | 02:18 |
| wallyworld_ | and it works just fine now | 02:18 |
| wallyworld_ | so we are causing a deliberate regresiion | 02:18 |
| wgrant | Has it ever been used? | 02:18 |
| wallyworld_ | in tests | 02:18 |
| wallyworld_ | i have no way of knowing whether on prod | 02:18 |
| wgrant | It wasn't used during the several week information_type transition on production. | 02:19 |
| wallyworld_ | ingrequent != won't ever | 02:19 |
| wallyworld_ | we should not allow the system to get into a bad state | 02:20 |
| wgrant | launchpad_dogfood=# SELECT COUNT(*) FROM branch WHERE stacked_on = (SELECT id FROM branch WHERE unique_name = '~launchpad-pqm/launchpad/db-devel'); | 02:20 |
| wallyworld_ | and that's what we wil be doing in introducing this regression in functionality | 02:20 |
| wgrant | count | 02:20 |
| wgrant | ------- | 02:20 |
| wgrant | 5018 | 02:20 |
| wgrant | (1 row) | 02:20 |
| wgrant | That's pretty unbounded. | 02:20 |
| wgrant | wallyworld_: What is wrong with the state? | 02:20 |
| wallyworld_ | 5018 < infinity | 02:20 |
| wallyworld_ | public branch stacked on private branch doesn't make sense | 02:21 |
| wgrant | It doesn't make sense, no. | 02:21 |
| wallyworld_ | and yet we will be allowing it | 02:21 |
| wgrant | But why do we need to prevent it? | 02:21 |
| wallyworld_ | because it doesn't make sense | 02:21 |
| wgrant | We should prevent things that are harmful. | 02:21 |
| wallyworld_ | and the whole reason transitively private was done was to stop this | 02:21 |
| wgrant | Not quite | 02:21 |
| wallyworld_ | that's the only reason transitivelt private was done | 02:21 |
| wgrant | transitively_private's main use case is handled by information_type | 02:22 |
| wgrant | That case is the one where a branch is pushed as public, but is stacked on a private branch. | 02:22 |
| wgrant | In that case the user is probably unknowingly making a mistake. | 02:22 |
| wgrant | We need to handle that, because it happens often due to misconfiguration. | 02:23 |
| wallyworld_ | and the other case will also happen | 02:23 |
| wgrant | The case that transitively_private handles now, but we are dropping with information_type, is where a branch and its stacked-on are public, and the stacked-on transitions to private. | 02:23 |
| wgrant | In that case the branch owner is already aware of some information disclosure. | 02:23 |
| wgrant | They're going to want to audit their project's sharing once they've fixed the immediate hole. | 02:23 |
| wallyworld_ | yes, and i totally disagree with that regresiion | 02:23 |
| wallyworld_ | it is insane we are allowing bad data from one direction and stopping it from the other | 02:24 |
| wgrant | Hm? | 02:24 |
| wgrant | We're not doing it to stop bad data. | 02:24 |
| wgrant | We're doing it to stop people from a footgun that they can't know about. | 02:24 |
| wallyworld_ | and yet we will allow it still | 02:25 |
| wgrant | No. | 02:25 |
| wallyworld_ | yes | 02:25 |
| wallyworld_ | change the stacked on to private and boom | 02:25 |
| wgrant | Boom? What boom? | 02:25 |
| wgrant | Nothing breaks. | 02:25 |
| wgrant | Some stuff may be left public. | 02:25 |
| wgrant | But it's not unknown. | 02:25 |
| wallyworld_ | what breaks is that people will want to work with the public branch but cant | 02:25 |
| wgrant | Public->private of a stacked-on happens as a result of an incorrect configuration being discovered. | 02:26 |
| wgrant | Ah! | 02:26 |
| wallyworld_ | because they can't see the stacked on private branch | 02:26 |
| wgrant | Now that's something we can't solve. | 02:26 |
| wgrant | They can't work with it either way. | 02:26 |
| wgrant | If it's public, they can no longer work with it since it's stacked on a private branch. | 02:26 |
| wgrant | If we make it private, they can longer work with it since it's private. | 02:26 |
| StevenK | 2012-06-06 23:26:57 DEBUG2 [PopulateSourcePackagePublishingHistoryPackageUpload] Done. 231995 items in 337 iterations, 807.064977 seconds, average size 688.415397 (287.45639486/s) | 02:26 |
| wgrant | So it's no more unusable this way. | 02:26 |
| wallyworld_ | yes, and so it should be marked private | 02:26 |
| StevenK | wgrant: ^ | 02:26 |
| wgrant | wallyworld_: Why? | 02:27 |
| wallyworld_ | so that people realise it's private. as i said, that why tp was introduced | 02:27 |
| wgrant | wallyworld_: This is not a regression which makes those forgotten branches less usable. And this is not a regression which prevents an audit from working, since the other branches will clearly show up as public too. | 02:27 |
| wgrant | wallyworld_: tp's main usecase is, as I said, to prevent the initial footgun case. | 02:27 |
| wallyworld_ | it is a regression which removes previously agreed useful behaviour | 02:28 |
| wgrant | *Not* to handle this change later. | 02:28 |
| wgrant | It's very expensive and probably not worthwhile behaviour. | 02:28 |
| wallyworld_ | and we will now be allowing the initial footgun again | 02:28 |
| StevenK | wgrant: And racy. | 02:28 |
| wgrant | No | 02:28 |
| wgrant | How? | 02:28 |
| wallyworld_ | by allowing a stacked on branch to be marked private | 02:28 |
| wgrant | We explicitly do handle the initial push case. | 02:28 |
| wgrant | That's not the initial footgun. | 02:28 |
| wallyworld_ | it's a way to create the problem without people knowing | 02:29 |
| wgrant | Making a branch private later is a very explicit operation. | 02:29 |
| wallyworld_ | and one which is easy to mess up | 02:29 |
| wallyworld_ | if you don't realise yor branch is stacked on | 02:29 |
| wgrant | Sure. | 02:29 |
| StevenK | Then let's warn in the UI and move on | 02:29 |
| wgrant | And what is the fallout from it? | 02:29 |
| wallyworld_ | we just simply cannot alllw this | 02:29 |
| wgrant | It is messy, but not a significant security issue. | 02:30 |
| wallyworld_ | ui != api | 02:30 |
| wgrant | So a warning is probably sufficient. | 02:30 |
| wallyworld_ | it's a significant functional issue | 02:30 |
| wgrant | In an extraordinarily rare situation. | 02:30 |
| wgrant | Where someone has already broken shit. | 02:30 |
| wallyworld_ | since it inadvertantly stops peope working wit the public branch | 02:30 |
| wgrant | Huh? | 02:30 |
| wgrant | That's irrelevant. | 02:30 |
| wallyworld_ | not rare enough that tp wasn't needed | 02:30 |
| wgrant | We'd just be advertantly stopping people from working with it. | 02:30 |
| wgrant | wallyworld_: tp solves the footgun case. | 02:30 |
| wgrant | This also solves the footgun case. | 02:30 |
| wallyworld_ | tp does more than that | 02:31 |
| wgrant | tp's implementation happens to also solve the later transition case | 02:31 |
| wallyworld_ | we are deliberately regressing | 02:31 |
| wgrant | This does not. It consciously does not. | 02:31 |
| wgrant | Correct. | 02:31 |
| wgrant | But it's not a substantial regression in any way. | 02:31 |
| wallyworld_ | not "happens", it was a key part of the implementation | 02:31 |
| wallyworld_ | it is a substantioal regrsiion | 02:31 |
| wallyworld_ | we must not allow users to do things whuch break the system | 02:31 |
| wgrant | It does not break the system. | 02:32 |
| wallyworld_ | it breaks the ability for people other than themselves to work wit the stacked branches | 02:32 |
| wgrant | I can make a public branch unusable in other ways. | 02:32 |
| wgrant | eg. by deleting the repository from it. | 02:32 |
| wallyworld_ | so? | 02:32 |
| wallyworld_ | 2 wrongs doesn't make a right | 02:32 |
| wgrant | And all it does is avert a tiny bit of user confusion. | 02:33 |
| wgrant | Since if we follow tp, we just make the branch even less usable. | 02:33 |
| wallyworld_ | it does a lot more than that | 02:33 |
| wgrant | Not more usable. | 02:33 |
| wallyworld_ | why is the branch less usable wit tp? | 02:33 |
| wgrant | wallyworld_: Your complaint is that with information_type the branch is unusable by people who don't have access to the stacked-on branch. | 02:34 |
| wallyworld_ | without tp, the ui lies | 02:34 |
| wgrant | wallyworld_: With tp the branch is just as unusable. | 02:34 |
| wgrant | Sure. | 02:34 |
| wgrant | But the UI can say "uh, your branch is stacked on a private one, but it's public. you're probably wrong" | 02:34 |
| wallyworld_ | but it the private branch could be several layers down | 02:34 |
| wgrant | That's true. | 02:35 |
| wallyworld_ | and model code also needs to treat the branch like it is private | 02:35 |
| wallyworld_ | to exlcude it from search results etc | 02:35 |
| wgrant | Why? | 02:36 |
| wgrant | It doesn't break anything. | 02:36 |
| wallyworld_ | if i change a staced on branch to private, any publiv branche sstaked on it must not show up in search results | 02:36 |
| wgrant | It just makes it a little confusing. | 02:36 |
| wgrant | Why not? | 02:36 |
| wgrant | https://pastebin.canonical.com/67615/ | 02:36 |
| wallyworld_ | because the stacked branch is to be considered private | 02:36 |
| wallyworld_ | that was the required tp functionality | 02:36 |
| wallyworld_ | when that work was speced out | 02:36 |
| wgrant | canonical-payment-service and ztrustee were misconfigurations that sinzui fixed. information_type handles those identically, because it's the initial footgun case. libunity-webapps was a misconfiguration that required a full project audit afterwards. Everything else has less than 5 branches in this state. | 02:37 |
| wgrant | Which proves that no large transition has ever made use of the tp ability that we're regressing on. | 02:37 |
| wgrant | Except *possibly* libunity-webapps. | 02:37 |
| wallyworld_ | sure, but just because it's uncommon, we can't just unilaterally decide to regress | 02:38 |
| wgrant | That paste covers every branch that has made use of tp, except for those where explicity_private was set afterwards. | 02:38 |
| wgrant | wallyworld_: We're regressing on a feature that has never been used. | 02:38 |
| wgrant | Not once. | 02:38 |
| wallyworld_ | what about libunity-webapps | 02:39 |
| wgrant | wallyworld_: What if we don't consider the stacked branch private? | 02:39 |
| wgrant | wallyworld_: What if we consider it misconfigured? | 02:39 |
| wallyworld_ | and it's not so much that it's used, it is designed to prevent inconsistent data | 02:39 |
| wallyworld_ | and confusion | 02:39 |
| wgrant | libunity-webapps I shall not go into here, but it was a complete misconfiguration of a private project. | 02:39 |
| wgrant | In the case of a misconfigured project like that one, you should be fully auditing +sharing anyway. | 02:40 |
| wallyworld_ | even if we consider it misconfisgured, why would we deliverately design out a saftely check to prevent the misvonfiguration in the first place? | 02:40 |
| wgrant | wallyworld_: It's not proven useful even once, it requires unbounded write cascades, and it is non-trivial to implement with information_type. | 02:41 |
| wallyworld_ | it was useful because of the state lp was in befre it was implemented | 02:41 |
| wgrant | The main usecase for tp is extremely useful. | 02:42 |
| wallyworld_ | there were a large number of public branches stacked on private | 02:42 |
| wgrant | This one is not. | 02:42 |
| wgrant | Sure. | 02:42 |
| wallyworld_ | and now we can have the all over again | 02:42 |
| wgrant | No! | 02:42 |
| wgrant | No! | 02:42 |
| wallyworld_ | yes | 02:42 |
| wgrant | Only if a project changes their stacked-on branch without touching the rest. | 02:42 |
| wallyworld_ | why do you say no when we have already agreed it can happen? | 02:43 |
| wgrant | In that case it's not a vulnerability. | 02:43 |
| wgrant | It's a project being stupid. | 02:43 |
| wallyworld_ | but they may no realise their branch is stacked on | 02:43 |
| wgrant | wallyworld_: As we've seen, the vast, vast majority of cases where it can happen is the initial push. | 02:43 |
| wgrant | So we won't have a large number of public branches stacked on private. | 02:43 |
| wallyworld_ | it can happen anytime | 02:43 |
| wgrant | We might have 1% of what we had before | 02:43 |
| wgrant | wallyworld_: Technically it can. | 02:43 |
| wgrant | Practically it won't unless the project owner is a moron. | 02:44 |
| wallyworld_ | and therefore it will | 02:44 |
| wgrant | Is it a problem? | 02:44 |
| wallyworld_ | moron != careless | 02:44 |
| wallyworld_ | yes | 02:44 |
| wallyworld_ | otherwise tp would not have been approved to be resourced | 02:44 |
| wgrant | Huh? | 02:44 |
| wgrant | TP solved a very real problem | 02:44 |
| wgrant | It was totally worth it | 02:44 |
| wgrant | It also solved another problem that wasn't real. | 02:44 |
| wgrant | We have hard data on that now. | 02:44 |
| wallyworld_ | it was rel | 02:45 |
| wallyworld_ | i can't recall the exact figures, but a garbo job was needed to clean up the data | 02:45 |
| wgrant | That is entirely irrelevant to the nature of that data. | 02:45 |
| wgrant | The volume and split are unrelated. | 02:45 |
| wgrant | The data I just obtained from dogfood confirms my assumptions: that the case we're dropping is unused. | 02:46 |
| wgrant | It's a case I was always suspicious about. | 02:46 |
| wallyworld_ | used now != used ever | 02:46 |
| wallyworld_ | and we are taking away a safety net | 02:46 |
| wallyworld_ | to prevent bad data | 02:46 |
| wallyworld_ | deliberately | 02:46 |
| wgrant | OK | 02:47 |
| wgrant | It's not bad data. | 02:47 |
| wallyworld_ | it is | 02:47 |
| wallyworld_ | because it causes confusion | 02:47 |
| wallyworld_ | and stuff to break | 02:47 |
| wgrant | Preventing bad data is not itself a useful goal | 02:47 |
| wallyworld_ | huh? | 02:47 |
| wgrant | The "bad data" has to have some negative effect | 02:47 |
| wallyworld_ | it does | 02:47 |
| wgrant | The goal is to eliminate the negative effect. | 02:47 |
| wgrant | Right | 02:47 |
| wallyworld_ | confusion, no longer being able to use the stacked branch | 02:47 |
| wgrant | So let's stop talking about bad data, and start talking about the negative effects :) | 02:47 |
| wgrant | Confusion: granted, we can fix that with messages on the branch page, as we do already if the stacked-on location is invalid. | 02:48 |
| wallyworld_ | or we could just prevent the bad data and not introduce a deliberate regresiion | 02:48 |
| wallyworld_ | we would need to drill down the entire stack | 02:48 |
| wgrant | No longer being able to use the stacked branch: that's a false statement; you can't use it if it's private either. | 02:48 |
| wallyworld_ | but it's not pretending to be usable | 02:49 |
| wgrant | So that goes back to confusion. | 02:49 |
| wgrant | Which is very similar to the invalid stacked-on issue | 02:49 |
| wgrant | For which we currently warn | 02:49 |
| wgrant | With a warning on Branch:+index | 02:49 |
| wallyworld_ | we never used to warn | 02:49 |
| wallyworld_ | is that new | 02:49 |
| wgrant | We've always warned. | 02:49 |
| wgrant | Invalid meaning not a valid URL, or the URL points to something that doesn't exist or is outside Launchpad | 02:50 |
| wgrant | Not meaning private | 02:50 |
| wgrant | Yet. | 02:50 |
| wallyworld_ | nope, not in the case of public stacked on private afair | 02:50 |
| wallyworld_ | we used to just show the stacked branch as if it were public | 02:50 |
| wgrant | 12:49:24 < wgrant> Which is very similar to the invalid stacked-on issue | 02:50 |
| wgrant | 12:49:27 < wgrant> For which we currently warn | 02:50 |
| wgrant | "is very similar to" | 02:51 |
| wgrant | Not "is" | 02:51 |
| wallyworld_ | so it's new then | 02:51 |
| wgrant | Hm? | 02:51 |
| wgrant | That warning is not new. | 02:51 |
| wgrant | Extending it to warn when the stacked-on is private would be new. | 02:51 |
| wallyworld_ | but we never used to warn people their public branch was stacked on a private one | 02:51 |
| wgrant | No. | 02:51 |
| wgrant | I never said we did. | 02:51 |
| wgrant | I'm saying it's what we should do | 02:52 |
| wallyworld_ | hence my assertion it will be new | 02:52 |
| wgrant | Sure, warning *on privacy violation* is new. | 02:52 |
| wgrant | Warning on bad stacked-on in general is not. | 02:52 |
| wgrant | So it's quite anomalous that we don't warn in the case of a privacy violation. | 02:52 |
| wallyworld_ | even recusive stacked on | 02:52 |
| wallyworld_ | ? | 02:52 |
| wgrant | I'm not sure about that. | 02:53 |
| wallyworld_ | we may warn on bad stacked on, but perhaps not recusrively | 02:53 |
| wallyworld_ | i guess it comes down to in part philosophy - should software prevent a known bad situation | 02:53 |
| wallyworld_ | or should software regress deliberately | 02:54 |
| wgrant | It's a tradeoff. | 02:54 |
| wallyworld_ | given the current implementation works fine, i'm not sure i see why we are doing this | 02:54 |
| wgrant | We can maintain messy code of unbounded computational complexity that will probably never be usefully used. | 02:54 |
| StevenK | wallyworld_: Are you done yelling at wgrant? :-) | 02:54 |
| wgrant | Or we can not. | 02:54 |
| wallyworld_ | it's not messy | 02:55 |
| wallyworld_ | the algorithm is very simple | 02:55 |
| wgrant | With transitively_private it's not messy. | 02:55 |
| wgrant | With information_type it is slightly moreso. | 02:55 |
| wgrant | Because it's not boolean. | 02:55 |
| wallyworld_ | so what i would like to see is an exception raised | 02:55 |
| wgrant | Hahahaha | 02:55 |
| wallyworld_ | if someone attempts to change a stacked on branch to an invalid state | 02:55 |
| wgrant | We can't do that every time. | 02:56 |
| wallyworld_ | why? | 02:56 |
| wgrant | You may recall we discussed this for weeks. | 02:56 |
| wgrant | The information_type of a stacked on branch could change directly. | 02:56 |
| StevenK | stacking can change via an sftp client | 02:56 |
| wgrant | Or it could change by its stacked-on branch changing. | 02:56 |
| wgrant | Precisely. | 02:56 |
| wallyworld_ | sure, but i'm talking about public stacked on not public | 02:56 |
| wgrant | So. | 02:56 |
| wgrant | Listen to this case: | 02:56 |
| wallyworld_ | via sftp we can't detect sure | 02:57 |
| wgrant | I have a stacking chain. A stacked on B stacked on C | 02:57 |
| wallyworld_ | but that's the case now with tp also | 02:57 |
| wgrant | They're all public. | 02:57 |
| wgrant | Now, the owner of B changes it to stack on D, a private branch. | 02:57 |
| wgrant | What happens now? | 02:57 |
| wgrant | We can't block that. | 02:57 |
| wgrant | So A and B have to become private, or we have to let them be inconsistent. | 02:58 |
| wgrant | Blocking is the ideal solution. | 02:58 |
| wgrant | But it's not feasible. | 02:58 |
| wgrant | So we have to either propagate or accept. | 02:58 |
| wallyworld_ | what happens now is A and B become private | 02:58 |
| wgrant | Yes. | 02:58 |
| wallyworld_ | as they should | 02:58 |
| wgrant | That is arguable. | 02:58 |
| wallyworld_ | the argument was had before tp was approved | 02:59 |
| wgrant | And even if we decide that they should, it becomes is it worth it. | 02:59 |
| wallyworld_ | and it came out in favour of tp | 02:59 |
| wgrant | Curtis agreed with my view on several calls, and I'm pretty sure you were there too :) | 02:59 |
| wallyworld_ | i'm not sure it was agreed to intorduce a deleibate regression | 03:00 |
| wgrant | It may have been agreed during TP's design on the absence of useful data. | 03:00 |
| wgrant | But we have useful data now. | 03:00 |
| wgrant | It was. | 03:00 |
| wgrant | It was acknowledged as a regression. | 03:00 |
| wgrant | And I now have data which proves beyond reasonable doubt that nobody will miss it, because nobody has used it. | 03:00 |
| wallyworld_ | it's not that nobody has used it i'm worried about - it's the ability to allow a misconfigured sysem, deliberately | 03:01 |
| wgrant | Sure | 03:01 |
| wgrant | It's less than ideal. | 03:01 |
| wallyworld_ | because nobody uses it is even more a case not to allow it | 03:01 |
| wgrant | But the misconfiguration is not fatal. | 03:01 |
| wallyworld_ | fsdo | 03:01 |
| wgrant | There are lots of other misconfigurations that we permit and can't reasonably forbid. | 03:01 |
| wgrant | And this misconfiguration is expensive to prevent. | 03:02 |
| wallyworld_ | well we can reasonably prevent this one because we already do | 03:02 |
| wgrant | Only in a way that will probably time out if it's ever used. | 03:02 |
| wallyworld_ | nope, it's used now | 03:02 |
| wgrant | On any project of significance. | 03:02 |
| wgrant | It's *NOT* | 03:02 |
| wgrant | The data I gave proves that. | 03:02 |
| wallyworld_ | the triggers are run everytime a branch changes | 03:02 |
| wgrant | Hmmmmmm? | 03:03 |
| wgrant | Yes. | 03:03 |
| wgrant | But we're not talking about the triggers' overall behaviour. | 03:03 |
| wgrant | We're talking about the particular aspect that we're dropping. | 03:03 |
| wgrant | The unbounded cascade from the lower branch. | 03:03 |
| wallyworld_ | i don't want to redo the cascade bit | 03:03 |
| wallyworld_ | just the bit to prevent bad data | 03:03 |
| wgrant | Hm? | 03:04 |
| wgrant | Confused | 03:04 |
| wgrant | The bit we're regressing on is the cascade bit. | 03:04 |
| wallyworld_ | to prevent changing a stacked on branch to userdata | 03:04 |
| wgrant | Ah | 03:04 |
| wallyworld_ | if the stacked branch is public | 03:04 |
| wgrant | Now, we could prevent that at the UI and API level. | 03:04 |
| wgrant | However, we can't prevent that at the bzr level. | 03:04 |
| wallyworld_ | yes, that's all i am wanting | 03:04 |
| wallyworld_ | sure | 03:04 |
| wgrant | OK, that is a more reasonable proposal. | 03:05 |
| wallyworld_ | we don't do anything at the bzr level now for tp | 03:05 |
| wgrant | We do | 03:05 |
| wallyworld_ | wthat's all i've been arguing for here | 03:05 |
| wgrant | When the stacked-on changes via bzr, it will change in the DB. | 03:05 |
| wgrant | And transitively_private will work | 03:05 |
| wgrant | It is no problem there because it's not trying to prevent the transition. | 03:05 |
| wgrant | Just react to it. | 03:05 |
| wallyworld_ | ah right. well we should do the same check in the new world also | 03:05 |
| wgrant | We don't want to react to it. | 03:05 |
| wgrant | wallyworld_: Which is exactly the cascading thing. | 03:06 |
| wgrant | We have three options: | 03:06 |
| wallyworld_ | i'm not talking about cascading any changes as we do now, just now allowing the transiitin | 03:06 |
| wallyworld_ | not | 03:06 |
| wgrant | 1) Do nothing. Let people shoot themselves in the foot if they shuffle branches around in strange ways. | 03:06 |
| wgrant | 2) Cascade privacy changes through the tree. | 03:07 |
| wgrant | 3) Block transitions that violate the rules. | 03:07 |
| wgrant | TP does (2) | 03:07 |
| wallyworld_ | i'm arguing for 3, and have been for 30 minutes | 03:07 |
| wgrant | information_type does (1) | 03:07 |
| wgrant | (3) is impossible, since we can't block bzr changes. | 03:07 |
| wallyworld_ | why can't we block the stacked on change? | 03:08 |
| wallyworld_ | does bzr write straight to the db? | 03:08 |
| wgrant | No, which is exactly the problem. | 03:08 |
| wgrant | It's FS access | 03:08 |
| wallyworld_ | when in that case the triggers won't run | 03:09 |
| wallyworld_ | so how are the stacked on changes propogated? | 03:09 |
| wgrant | The branch scanner. | 03:09 |
| wgrant | After the push has completed. | 03:09 |
| wallyworld_ | so the branch scanner could reject the change | 03:09 |
| wgrant | So we cannot error at the time the client sets stacked-on | 03:09 |
| wgrant | Reject how? | 03:09 |
| wgrant | We could fail the scan. | 03:10 |
| wgrant | But you'll never guess what that does. | 03:10 |
| wallyworld_ | raise an oops? | 03:10 |
| wgrant | It leaves the branch public, but puts a warning on its page :) | 03:10 |
| wgrant | Which is exactly what I proposed. | 03:10 |
| lifeless | wgrant: you can block stacked on changes, just reject the IO operation | 03:10 |
| wallyworld_ | leaves the stacked branch public? | 03:10 |
| wgrant | lifeless: lol | 03:10 |
| wgrant | lifeless: I don't feel like rewriting bzr lp-serve today | 03:11 |
| lifeless | wgrant: why would you need to rewrite it? | 03:11 |
| lifeless | wgrant: it has a lp VFS module already | 03:11 |
| lifeless | wgrant: I'm not saying you should, I'm disputing your assertion that its impractical. | 03:12 |
| wgrant | lifeless: If it's practical to do that (eg. due to restrictions around RPC from the lp-serve process), why wasn't that done from the start instead of doing it at scan time which is clearly inferior? | 03:12 |
| lifeless | wgrant: what restrictions, lp-serve makes RPC calls all the time | 03:12 |
| wgrant | lifeless: It's possible. | 03:12 |
| wgrant | It's probably not practical. | 03:12 |
| mwhudson | it's not scan time | 03:12 |
| mwhudson | it's when the branch is unlocked | 03:13 |
| wgrant | mwhudson: branchChanged time, sorry | 03:13 |
| wgrant | After the end of the operation. | 03:13 |
| mwhudson | (which is admittedly too late to tell the writer to bugger off) | 03:13 |
| wgrant | Precisely. | 03:13 |
| lifeless | wgrant: there was no initial requirement to patrol stacking | 03:13 |
| lifeless | wgrant: you are adding one, which implies change. | 03:13 |
| lifeless | wgrant: so why wasn't it done this way before, because it wasn't done before at all. | 03:14 |
| wallyworld_ | and a change i think we should do | 03:14 |
| wgrant | lifeless: We break branches in LP when the stacked-on is invalid. | 03:14 |
| wgrant | That's not new functionality. | 03:14 |
| wgrant | wallyworld_: Why? | 03:14 |
| wgrant | I'm pretty sure it's completely not worth the extra code. | 03:14 |
| lifeless | wgrant: what do you mean break them ? | 03:14 |
| wallyworld_ | do i really have to explain myself all over again? | 03:14 |
| wgrant | And the data confirms it. | 03:14 |
| wgrant | It's an extraordinarily rare situation. | 03:15 |
| wgrant | And it will become even rarer once people can't break private project setup. | 03:15 |
| wallyworld_ | rare != we shouldn't prevent it | 03:15 |
| wgrant | Nothing goes terribly wrong if we don't prevent it. | 03:15 |
| wgrant | And preventing it requires lots of extra code. | 03:15 |
| wallyworld_ | lots? | 03:16 |
| wgrant | difficult to implement + very rare + largely inconsequential == not worth it | 03:16 |
| lifeless | wgrant: what do you mean break them ? | 03:16 |
| wgrant | lifeless: It stops scanning and throws warnings on the branch page. | 03:16 |
| wgrant | lifeless: Which is pretty unpleasant if it could just reject the push instead. | 03:16 |
| lifeless | it can | 03:18 |
| lifeless | however thats not break-the-branch, its aknowledge-that-its-broken | 03:18 |
| lifeless | we're not altering the branch data are we? | 03:18 |
| lifeless | wgrant: wallyworld_: Can I ask, whats the context - you have a long and interesting discussion, but whats the core issue ? | 03:18 |
| wallyworld_ | lifeless: we now handle the case if a stacked on branch is changed from public -> private | 03:19 |
| wallyworld_ | any stacked branches will become private also | 03:19 |
| wgrant | recursively | 03:19 |
| wallyworld_ | but the new world will see use allow a user to change a stacked on branch to private will no warning and the stacked branches will remain public | 03:20 |
| wallyworld_ | and the stacked branches will become unusable | 03:20 |
| wgrant | The stacked branches will become unusable to unprivileged users, same as they would be if they were made private. The only issue is confusion if the branch says it's public but isn't usable. | 03:21 |
| wallyworld_ | so we are allowing a footgun with no warning | 03:21 |
| lifeless | hmm | 03:21 |
| wgrant | I gathered evidence from the database that this feature hasn't been used. | 03:21 |
| lifeless | so, I would say, that changing other peoples branches to private is a misfeature | 03:21 |
| wallyworld_ | i'm not arguing it hasn't been used, i'm worried about the footgun | 03:22 |
| lifeless | wallyworld_: its fixable by the same user right ? | 03:22 |
| wallyworld_ | ie allowing someone to do something dumb with no warning | 03:22 |
| wallyworld_ | yes | 03:22 |
| lifeless | so the footgun specifically, is it: | 03:22 |
| wallyworld_ | fsvo fixabe | 03:22 |
| lifeless | a) the user changes the privacy setting for the stacked-on branch to private | 03:23 |
| lifeless | or | 03:23 |
| lifeless | b) in a public branch they set a stacked-on which is private | 03:23 |
| lifeless | ? | 03:23 |
| wallyworld_ | a) | 03:23 |
| wallyworld_ | we handle b) | 03:23 |
| wallyworld_ | and it's a regresion because we handle a) now | 03:23 |
| lifeless | with the general case of c) being 'setting a stacked-on which has a different visibility rule' (and applies if per-artifact access is given to a branch but not its stacked-on) | 03:23 |
| lifeless | do we handle c)? how do we handle it ? | 03:24 |
| wallyworld_ | triggers | 03:24 |
| wallyworld_ | transitively private | 03:24 |
| lifeless | I thought that column was deleted ? | 03:24 |
| wallyworld_ | but it's complicated with info type | 03:24 |
| lifeless | (being deleted) | 03:24 |
| wgrant | lifeless: It's about to be deleted. | 03:25 |
| wallyworld_ | yes, and so we need to figure out how to prevent the footgun | 03:25 |
| lifeless | so transitively private propogates outwards | 03:25 |
| wallyworld_ | if you mean up the stack , yes | 03:25 |
| lifeless | a) is a propogates-outward case, c) is a propogates inwards case. | 03:25 |
| lifeless | outward == up the stack, yes. | 03:25 |
| wallyworld_ | yes | 03:25 |
| wgrant | information_type is currently independent, except that if you push a new public branch and it's stacked on a private one, then you're probably doing something stupid and disclosing previously private code, so we make it private. | 03:25 |
| wallyworld_ | but we can't propogate info type | 03:25 |
| wallyworld_ | we should just prevent bad things | 03:25 |
| wallyworld_ | ie veto the info type change | 03:26 |
| wallyworld_ | or stacking change | 03:26 |
| lifeless | ok, so I'm much more worried about c than a. | 03:26 |
| wgrant | With the propagation behaviour we're not disclosing previously private code. | 03:26 |
| wgrant | We're just not automatically unleaking it. | 03:26 |
| wgrant | Um, with*out* the propagation behaviour | 03:26 |
| lifeless | a) very very few users will run into and they can restore functionality by unprivating the branch; or they can file a ticket to get a mass-migration of information type done (assuming they don't own all the branches) | 03:27 |
| lifeless | c) users will run into all the time, because feature branches are fairly common, and contractors are also fairly common | 03:28 |
| wgrant | This is upsetting | 03:28 |
| wgrant | I don't get to argue with lifeless this time :( | 03:28 |
| wallyworld_ | and c) is handled now but soon won't be | 03:28 |
| lifeless | wgrant: see, sometimes you are right ;) | 03:28 |
| wgrant | Hm? | 03:28 |
| wallyworld_ | my view is that we should not allow the users to enter bad data | 03:28 |
| wgrant | Isn't (c) the case we *do* handle? | 03:28 |
| wallyworld_ | if a user changes a branch to stack on a private one, the any public branches stacked on top "break" | 03:29 |
| wgrant | That's case (a) | 03:29 |
| wallyworld_ | with transitively private, they didn't | 03:29 |
| wallyworld_ | a) is changing the privacy, not the stacked on | 03:30 |
| wallyworld_ | or am i wrong | 03:30 |
| lifeless | ah, I think the phrasing is poor. | 03:30 |
| lifeless | let me rephrase. | 03:30 |
| lifeless | General case there are three branch sets to consider. | 03:30 |
| lifeless | stacked-on branche(s) - its transitive following the pointers. | 03:31 |
| lifeless | <the branch> | 03:31 |
| lifeless | stacking-on-this branch(es) - anything that stacks on this branch, or transitively outwards | 03:31 |
| wgrant | wallyworld_: Ah right, misunderstood you. | 03:32 |
| wgrant | wallyworld_: Right, if the change an intermediate branch it still breaks. | 03:32 |
| wallyworld_ | yes | 03:32 |
| lifeless | changing the pointer in <the branch> to point to a branch with different visibility rules (e.g. different info type) is equivalent to to changing the visibility rule on a member of the stacked-on branch set. | 03:32 |
| lifeless | in terms of impact on the stacking-in-this branches. | 03:33 |
| lifeless | The difference is whether its web ui (change info type) or bzr (change stacking pointer) | 03:33 |
| lifeless | from the perspective of any member of stacking-on-this, its identical. | 03:33 |
| lifeless | ^ does this make sense ? | 03:33 |
| wgrant | Yes | 03:33 |
| wallyworld_ | yes | 03:34 |
| wallyworld_ | we can detect that we are going to stack onto an incompatible branch | 03:34 |
| wallyworld_ | but we are saying that we will not detect changing a branch which will cause outward brancvhes to become invalid | 03:34 |
| lifeless | and we can detect when changing info type that there is a branch stacked on this that is incompatible (again transitively) | 03:34 |
| wallyworld_ | yes | 03:34 |
| lifeless | so, what user stories do we have around this | 03:35 |
| wallyworld_ | but we the proposal is not to do that anymore | 03:35 |
| lifeless | one story is 'I have a branch that should be different info-type *and* it has stacked-on branches that I do not control' | 03:35 |
| wallyworld_ | or 'has stacked on branches i do not know about' | 03:36 |
| lifeless | right | 03:36 |
| wallyworld_ | hence i may break them inadvertantly | 03:36 |
| wallyworld_ | with no warning | 03:36 |
| lifeless | so what can we do here | 03:39 |
| lifeless | we could stop the user changing the setting (be that stacked-on *or* information-type = we've established identity) | 03:39 |
| wallyworld_ | yes | 03:40 |
| wallyworld_ | so with transitively private, we allowed the change because it was transparently propogated | 03:40 |
| wallyworld_ | but now it won't be | 03:40 |
| lifeless | we could allow the branches that are no longer compatible [for users that don't meet the union of visibility rules] to become inoperable for those users | 03:40 |
| lifeless | we could overlay the new information type into all the affected branches [which is what transitively private did] | 03:41 |
| wallyworld_ | ipractical | 03:41 |
| wallyworld_ | with allowing the branches to become inoperable, i'm arguing we should warn the user first | 03:42 |
| lifeless | here is my suggestion | 03:42 |
| wallyworld_ | via the api, not sure how to handle | 03:42 |
| lifeless | I think you should just do the change requested. | 03:42 |
| lifeless | tell the user the *impact* of the change. | 03:42 |
| lifeless | They can switch it back in a heartbeat if they made a mistake, API *or* web UI. | 03:43 |
| lifeless | or bzr for that matter. | 03:43 |
| wallyworld_ | that works i think, at least then it's an informed decision, not something done that they dont know about, and that was my main issue | 03:43 |
| lifeless | I totally agree that silent breakage at a temporal distance from the act is terrible | 03:44 |
| lifeless | this will avoid that; we can, if you desire, send a warning to bzr too. | 03:44 |
| wallyworld_ | yes, and this is what i was arguing against | 03:44 |
| lifeless | I think though, that anyone doing bzr set-stacked manually, is in a class of their own : I wouldn't bother handling it. | 03:44 |
| lifeless | wallyworld_: this == 'silent breakage at a temporal distance from the act is terrible' ? | 03:45 |
| wallyworld_ | yes | 03:45 |
| lifeless | righto | 03:45 |
| lifeless | wgrant: ^ what do you think of my proposal? | 03:45 |
| wallyworld_ | i have no problem with the new limitations in behaviour | 03:46 |
| wgrant | We can't forbid, but we can warn on the branches at both ends, which is my preference. | 03:46 |
| wallyworld_ | but the 'silent' breakage | 03:46 |
| wallyworld_ | we can forbid it, no? | 03:46 |
| wallyworld_ | sorry, i mean warn | 03:46 |
| lifeless | wallyworld_: so, the silent-breakage via bzr case is excruciatingly rare | 03:46 |
| wallyworld_ | after they have done it | 03:46 |
| lifeless | wallyworld_: bzr makes it super hard to change | 03:46 |
| lifeless | wallyworld_: and lp sets the default behavior (to the branch of the default series) | 03:47 |
| wallyworld_ | no problem there. but we can reasonably handle the other cases | 03:47 |
| wallyworld_ | and i would argue that peope doing it via bzr are more expert anyway | 03:47 |
| lifeless | warning in the web ui is easy; the API can return something with a warning too I expect. | 03:47 |
| wallyworld_ | yes | 03:47 |
| wgrant | The API can't return a warning that people will see. | 03:48 |
| wgrant | But in the web UI we can. | 03:48 |
| wallyworld_ | in the ui, warn as it is done as also after wards | 03:48 |
| lifeless | wgrant: OTOH folk doing API scripting are probably changing every branch they have. | 03:48 |
| lifeless | wgrant: e.g. the warning would be superfluous | 03:48 |
| wgrant | wallyworld_: Well | 03:48 |
| wgrant | wallyworld_: The branch edit page returns to the branch index page on submission. | 03:48 |
| wgrant | wallyworld_: So a bright warning at the top of the branch index page serves both purposes :) | 03:49 |
| wallyworld_ | we can ask for a confirmation | 03:49 |
| lifeless | wallyworld_: nix on that | 03:49 |
| wallyworld_ | in the web ui | 03:49 |
| wgrant | I don't think we should. | 03:49 |
| wgrant | It's trivial to revert | 03:49 |
| lifeless | wallyworld_: confirmations for things that are easily reversible is an antipattern of usability. | 03:49 |
| wgrant | And more expensive to add confirmation | 03:49 |
| wallyworld_ | ok | 03:49 |
| wgrant | Like | 03:49 |
| wgrant | It'll be a matter of clicking the edit icon | 03:49 |
| wgrant | and clicking Public in the picker | 03:49 |
| wallyworld_ | with the api, we could introduce a 'i really want to do this parameter' | 03:49 |
| wallyworld_ | so it raises an exception unless that parameter is set | 03:50 |
| wallyworld_ | that way, it's an informed choice | 03:50 |
| wallyworld_ | since that is a case of breakage from a distance | 03:50 |
| lifeless | ugh | 03:52 |
| StevenK | wallyworld_: Can you peer at http://pastebin.ubuntu.com/1027943/ ? | 03:52 |
| lifeless | expose the attribute as readonly, use an API call to set it rather than direct mutation, and have the return value describe the impact. | 03:52 |
| lifeless | wallyworld_: ^ | 03:52 |
| wallyworld_ | lifeless: that works for me | 03:52 |
| wallyworld_ | so long as the impact is communicated | 03:52 |
| lifeless | wallyworld_: wgrant: making a branch *public* that wasn't before is something that you might justify a confirmation for. | 03:53 |
| wallyworld_ | StevenK: looking | 03:53 |
| lifeless | because unhiding previously hidden data is infeasible | 03:53 |
| wgrant | lifeless: Sure. | 03:53 |
| wgrant | lifeless: But that is precisely the opposite of this case. | 03:53 |
| wgrant | lifeless: That one is a general issue with the information type picker, unrelated to stacking. | 03:53 |
| lifeless | actually, its tightly related :P | 03:53 |
| StevenK | Hmm | 03:53 |
| wgrant | Oh? | 03:53 |
| lifeless | which is that fixing a public->private change involves a private->public change :) | 03:54 |
| wgrant | Uh | 03:54 |
| wgrant | yes | 03:54 |
| wgrant | But that's barely relevant. | 03:54 |
| wgrant | It's a special case that will be fixed by the general fix. | 03:54 |
| wgrant | There's nothing even really special about it :) | 03:54 |
| StevenK | Lots of test fallout of forbidding open/delegated teams to subscribe to private branches. :-( | 03:55 |
| wgrant | Yeah | 03:55 |
| wgrant | We really need to split the two types of teams up better. | 03:56 |
| wgrant | So they're more obviously different types of objects. | 03:56 |
| StevenK | I think ec2 has found most of them | 03:56 |
| wgrant | makeTeam and makeBadgeSeekersGroup | 03:56 |
| StevenK | Hahaha | 03:56 |
| StevenK | Oh, of course it has, the instance has been up for 4:04 | 03:56 |
| StevenK | wgrant: Some of the fixes are trivial | 03:57 |
| wgrant | All of them should be. | 03:57 |
| StevenK | test_branchnamespace breaks in strange ways | 03:57 |
| StevenK | (When I make the 3 teams MODERATED) | 03:57 |
| wallyworld_ | StevenK: i'm not sure what caused that error ottomh | 03:57 |
| wgrant | Hmm | 03:57 |
| wallyworld_ | i'd need to debug it | 03:58 |
| wgrant | StevenK: What's the diff? | 03:58 |
| StevenK | wgrant: https://code.launchpad.net/~stevenk/launchpad/branch-subscribe-aag/+merge/108881 is the MP | 03:59 |
| wgrant | StevenK: So, the branch owner is trying to subscribe a private team that they can't see. | 03:59 |
| wgrant | I'm surprised that ever worked. | 04:00 |
| wgrant | The test is clearly on crack. | 04:00 |
| StevenK | There are a *bunch* of them | 04:00 |
| wgrant | Replace person_logged_in(...) with admin_logged_in() | 04:00 |
| wgrant | Problem solved | 04:00 |
| wgrant | (the write permission is not relevant in that test) | 04:00 |
| * wallyworld_ has to do school pickup | 04:01 | |
| StevenK | It isn't relevant to check that the person subscribing can see the team? | 04:01 |
| wgrant | StevenK: Look at the test. | 04:01 |
| wgrant | StevenK: It is testing that an anonymous user can't see a private subscriber. | 04:01 |
| wgrant | Not that a private subscriber can or can't be created. | 04:01 |
| wgrant | So the principal behind the creation is irrelevant. | 04:02 |
| StevenK | wgrant: That issue can't affect prod? | 04:03 |
| wgrant | StevenK: Only if someone is subscribing someone that they don't have View on, which I don't think is allowed since it discloses the team name. | 04:04 |
| wgrant | But it's possible that someone with LimitedView can subscribe, I suppose. | 04:05 |
| StevenK | It will oops | 04:05 |
| StevenK | So we can find out | 04:05 |
| wgrant | Right. | 04:05 |
| StevenK | Right, 18 failures, let's see how few I fixed | 04:05 |
| StevenK | Hmmmmm. A test creates a private team and then creates a branch with that owner and it blows up too. | 04:09 |
| wgrant | StevenK: The branch owner is irrelevant. | 04:10 |
| wgrant | StevenK: The current principal is important. | 04:10 |
| StevenK | It just calls into the factory twice, I'm not sure which principal is involed. | 04:11 |
| StevenK | *involved | 04:11 |
| StevenK | wgrant: I've thought a problem -- create a project and have an open team as a reviewer. Create a private branch and propose it. Boom. | 04:20 |
| wgrant | StevenK: Reviewers aren't subscribed unless the branch is private. | 04:21 |
| wgrant | And that will largely go away now we have APGs. | 04:21 |
| StevenK | wgrant: Yes, and you can't subscribe an open team to a private branch ... | 04:22 |
| StevenK | And if the reviewer is an open team ... | 04:22 |
| wgrant | StevenK: Right, that's stupid and should be forbidden :) | 04:23 |
| wgrant | It doesn't make sense. | 04:23 |
| wgrant | Forbidding something that shouldn't be permitted is not a bug :) | 04:23 |
| StevenK | Yes, so I was going to chase that rabbit hole for a bit. | 04:23 |
| StevenK | Trying to figure out where reviewers are in the DB | 04:24 |
| StevenK | Hmmm, can't see it in the UI either currently, that's disappointing | 04:33 |
| wgrant | What do you mean? | 04:33 |
| StevenK | wgrant: So when I propose a branch for LP, a team is set as the reviewer. I'm trying to figure out where that is in the DB. | 04:34 |
| wgrant | StevenK: branch.reviewer | 04:35 |
| StevenK | I discounted that because I thought it was per-branch | 04:36 |
| wgrant | It is per-branch. | 04:37 |
| wgrant | The default reviewer for an MP is the target branch's default reviewer. | 04:37 |
| StevenK | Which is in .reviewer for that branch, right. | 04:38 |
| StevenK | wallyworld: ^ Distracted from my other branch by a different problem. | 04:54 |
| StevenK | People that have set an open team as a reviewer need to be shot. | 04:55 |
| wallyworld | StevenK: i think so | 04:55 |
| StevenK | wallyworld: About which? | 04:56 |
| wallyworld | the default mp reviewer is the branche's default reviewer | 04:56 |
| lifeless | StevenK: again, its our oversight | 05:50 |
| lifeless | s/again, // | 05:50 |
| StevenK | lifeless: Which is? | 05:50 |
| lifeless | failing to consider the implications of open teams on integrity | 05:52 |
| StevenK | Yup. | 05:53 |
| StevenK | I'm going to bring it up on the call tomorrow. | 05:53 |
| lifeless | StevenK: you're aware of all the other cases we've fixed so far? | 06:00 |
| lifeless | StevenK: we should probably just do an audit | 06:00 |
| lifeless | ppas, project roles, .... | 06:00 |
| StevenK | lifeless: I was involved in both of them closely. | 06:03 |
| StevenK | lifeless: However, these situations are not as cut-n-dried as PPAs and project roles. | 06:05 |
| StevenK | There are arguments both ways, which there wasn't for the two earlier cases. | 06:06 |
| wallyworld | wgrant: https://code.launchpad.net/~wallyworld/launchpad/sharing-job-fflag-protection/+merge/109072 | 06:12 |
| lifeless | StevenK: well, there weren't for ppas, because sudo on machines, but the rest there are :) | 06:50 |
| lifeless | StevenK: if you consider tarmac, this case becomes roughly === ppas | 06:50 |
| wgrant | lifeless: branch.reviewer is different, since most projects don't use Tarmac, so there are uses where an open team doesn't mean root for everyone. | 06:55 |
| wgrant | lifeless: For PPAs there's no use that doesn't mean root for everyone. | 06:56 |
| wgrant | wallyworld: That's the third feature flag for the same thing. | 06:57 |
| wallyworld | not really | 06:57 |
| wallyworld | i reused one | 06:57 |
| wgrant | wallyworld: All the other stuff is guarded by one of the two feature flags that you introduced two days ago. | 06:57 |
| wallyworld | for this | 06:57 |
| wgrant | And, critically, we don't want to turn writing on before we confirm that these work on production. | 06:57 |
| wallyworld | different semantics | 06:58 |
| wgrant | So turning this on only when we turn writing on is less than ideal. | 06:58 |
| === almaisan-away is now known as al-maisan | ||
| wgrant | I'd have all the job creation under a flag like disclosure.that_job_stuff.enabled | 06:58 |
| wgrant | Rather than 3 with misleading names. | 06:58 |
| wallyworld | misleading? | 06:59 |
| wallyworld | to whom? | 06:59 |
| wallyworld | for this branch, i just wanted a fast way to disable the jobs | 06:59 |
| wallyworld | so we can deploy | 06:59 |
| wallyworld | without having to add another flag | 07:00 |
| wallyworld | the closest existing flag is perhaps disclosure.legacy_subscription_visibility.enabled but that one is for when we want unsubscribing to revoke access. the jobs are for the opposite direction so different | 07:02 |
| StevenK | lifeless: Tarmac and BMQs are a compelling argument indeed. | 07:02 |
| wallyworld | wgrant: so which flag did you want me to use if you disagree with the one i chose. remember i just want something quick to stop the jobs running on prod | 07:09 |
| wgrant | wallyworld: I'd probably just use disclosure.legacy_subscription_visibility.enabled. But I guess we use writable temporarily just to disable it, you're right. | 07:13 |
| wallyworld | i can clean up next branch | 07:13 |
| wallyworld | i just want to unblock deployment | 07:13 |
| wallyworld | which has been blocked for over a day :=( | 07:13 |
| wgrant | wallyworld: I'm on a call right now, but is that all the callsites for both types of jobs? | 07:14 |
| wallyworld | wgrant: unless i missed anything but i double checked | 07:15 |
| wallyworld | there are 2 places in the service but the methods they are inside are protected | 07:16 |
| wgrant | wallyworld: r=me, thanks. | 07:16 |
| wallyworld | thank you | 07:16 |
| === al-maisan is now known as almaisan-away | ||
| lifeless | wgrant: can you remind me tomorrow to send a summary to l-dev ? | 09:04 |
| wgrant | lifeless: Sure. | 09:06 |
| lifeless | also ahahaha https://lp-oops.canonical.com/oops.py/?oopsid=61493187d8286292a72f12956de8b3cb#longstatements | 09:06 |
| lifeless | branch fti query strikes again | 09:07 |
| lifeless | on +addseries | 09:07 |
| * czajkowski peers at wgrant are you doing stuff to lp atm | 09:07 | |
| wgrant | lifeless: Ew | 09:07 |
| lifeless | https://github.com/cjdelisle/cjdns/blob/master/rfcs/Whitepaper.md | 09:07 |
| lifeless | elmo: ^ may make you laugh, may make you cry | 09:07 |
| czajkowski | https://lp-oops.canonical.com/oops.py/?oopsid=61493187d8286292a72f12956de8b3cb am getting these while doing translations imports | 09:07 |
| === almaisan-away is now known as al-maisan | ||
| jml | hello | 10:22 |
| jml | I'd like to QA https://bugs.launchpad.net/launchpad/+bug/1006295 | 10:23 |
| _mup_ | Bug #1006295: Archive's 'suppress_subscription_notifications' attribute is called 'commercial' in the database <qa-needstesting> <tech-debt> <Launchpad itself:In Progress by jml> < https://launchpad.net/bugs/1006295 > | 10:23 |
| jml | I *think* the right thing to do is to set the attribute on an archive on qastaging and then perhaps check that 'commercial' was set in the database | 10:24 |
| jml | can someone confirm? | 10:24 |
| wgrant | jml: I'd just check that you can set it and read it. | 10:28 |
| wgrant | If it reads back correctly, it probably wrote to the database, and probably to the commercial column :) | 10:29 |
| jml | ok. | 10:29 |
| jml | I guess I'll just use one of my own crappy testing PPAs | 10:29 |
| jml | Python sucks. | 10:35 |
| jml | Let's rewrite everything in Clojure. | 10:36 |
| wgrant | Good plan. | 10:36 |
| jml | You know what else sucks? | 10:36 |
| jml | transparent network operation syntax | 10:36 |
| wgrant | Indeed | 10:40 |
| jml | Hmm. | 10:42 |
| jml | neither suppress_subscription_notifications nor commercial appear in the PPA dict for me (for a PPA I own) | 10:42 |
| wgrant | jml: Are you sure you're using the right API version? | 10:43 |
| jml | wgrant: no. | 10:43 |
| wgrant | It's probably only exported in devel, not 1.0 | 10:43 |
| jml | Hmm. | 10:43 |
| wgrant | Launchpad.login_with('fooconsumer', 'qastaging', version='devel') | 10:44 |
| jml | wgrant: thanks. | 10:45 |
| jml | wgrant: hmm. no change. | 10:45 |
| wgrant | jml: Perhaps you need to remove your WADL cache | 10:52 |
| wgrant | In ~/.cache/launchpadlib or ~/.launchpadlib | 10:52 |
| jml | wadl. cache. | 10:53 |
| * jml parties like it's 1999 | 10:53 | |
| wgrant | It's on https://api.qastaging.launchpad.net/devel/~wgrant/+archive/ppa, so the server knows about it... | 10:53 |
| wgrant | Heh | 10:53 |
| jml | HHmm. | 10:56 |
| jml | getPPAByName doesn't work the way I think it should. | 10:57 |
| jml | Or maybe createPPA doesn't. | 10:58 |
| wgrant | jml: Are you calling lp.me.somemethod? | 10:58 |
| jml | wgrant: yes. | 10:58 |
| wgrant | That doesn't work | 10:58 |
| jml | ffs | 10:58 |
| wgrant | It'll just return the person instead. | 10:58 |
| jml | thanks. | 10:58 |
| jml | I'd noticed. | 10:58 |
| wgrant | lp.people[lp.me.name].somemethod | 10:58 |
| wgrant | will work | 10:58 |
| wgrant | If you're not dead yet. | 10:59 |
| jml | \o/ | 11:01 |
| jml | wgrant: sanity check this script for me? | 11:01 |
| jml | http://paste.ubuntu.com/1028403/ | 11:01 |
| jml | output: http://paste.ubuntu.com/1028404/ | 11:01 |
| jml | is there a way I can get an alert when a revno gets deployed? | 11:07 |
| wgrant | jml: I can comment in the bug. | 11:09 |
| jml | wgrant: thanks. | 11:09 |
| wgrant | But probably in about 13 hours. | 11:09 |
| jml | cool. | 11:12 |
| === matsubara is now known as matsubara-lunch | ||
| stub | wgrant: Did you see anything horrible happen with the GIN index on staging? | 11:35 |
| wgrant | stub: It seems to not be broken. | 11:37 |
| stub | Might do the rest of them then | 11:37 |
| === rick_h changed the topic of #launchpad-dev to: http://dev.launchpad.net/ | On call reviewer: - | Firefighting: - | Critical bugs: 3.47*10^2 | ||
| === matsubara-lunch is now known as matsubara | ||
| jml | hello | 13:10 |
| jml | can someone in ~launchpad please claim a number for me in lp:~launchpad/+junk/dbpatches | 13:10 |
| rick_h | looking jml | 13:11 |
| jml | rick_h: thank you. | 13:12 |
| rick_h | jml: looks like I need a comment for it? | 13:12 |
| jml | rick_h: Rename Archive.commercial to Archive.suppress_subscription_notifications (bug #1006295) | 13:13 |
| _mup_ | Bug #1006295: Archive's 'suppress_subscription_notifications' attribute is called 'commercial' in the database <qa-ok> <tech-debt> <Launchpad itself:In Progress by jml> < https://launchpad.net/bugs/1006295 > | 13:13 |
| rick_h | jml: ok, think I got that | 13:18 |
| jml | rick_h: thanks. | 13:19 |
| jml | I got a lot of spew when I ran 'make newsampledata': http://paste.ubuntu.com/1028563/ | 13:22 |
| jml | mostly of the form, "NOTICE: there are circular foreign-key constraints among these table(s)" | 13:23 |
| jml | is this my fault? | 13:23 |
| jml | Also, there appear to be changes to ArchivePermission and SPPH after running 'make newsampledata'. I wasn't expecting those. Should I not be doing this from db-devel? | 13:28 |
| rick_h | sorry jml, no idea there. Have yet to do a db patch myself. stub is the master, you know? | 13:28 |
| === jcsackett changed the topic of #launchpad-dev to: http://dev.launchpad.net/ | On call reviewer: jcsackett | Firefighting: - | Critical bugs: 3.47*10^2 | ||
| rick_h | oh hai jcsackett | 13:31 |
| jcsackett | morning, rick_h. | 13:32 |
| rick_h | morning, -ops | 13:32 |
| * jcsackett idly wonders if the 1400 line diff in the MP queue is a harbinger of the day to come. | 13:32 | |
| * jml sends an email | 13:39 | |
| stub | jml: You can ignore that I think. That make sampledata stuff is an old hack and has never been pretty. | 13:48 |
| jcsackett | sinzui: i need a refresher from last night. which of the many cards was the one wallyworld was looking at you suggested i try for today? my mind is sort of mush, evidently. | 15:02 |
| sinzui | jcsackett, https://bugs.launchpad.net/launchpad/+bug/999596 | 15:03 |
| jcsackett | sinzui: ah, dig. yes, i should totally be able to fit that in today. | 15:03 |
| jcsackett | huh. looks like william is already doing it though. | 15:04 |
| sinzui | jcsackett, I think the complication is that the vocabs and personset have competing implementation | 15:04 |
| sinzui | jcsackett, that is my mistake. No one is working on it | 15:05 |
| jcsackett | sinzui: dig. i'll assign myself then. | 15:05 |
| cjwatson | oww, soyuz/doc/archive.txt makes me feel physically ill | 15:06 |
| cjwatson | 2762 lines of solid doctest | 15:06 |
| sinzui | cjwatson, I bet half of it is tested in a unit test, but removing the duplicate parts topple the doctest | 15:08 |
| cjwatson | Bingo | 15:08 |
| cjwatson | And so it requires actual thought to dissect. Grr | 15:09 |
| * jml wants to take a swipe at it. | 15:12 | |
| cjwatson | I'm at -253 on it so far | 15:13 |
| cjwatson | Though I strongly suspect there'll still be some left when I'm done | 15:13 |
| bdmurray | Could I get my merge proposal review? https://code.launchpad.net/~brian-murray/launchpad/bug-912137/+merge/109045 | 15:14 |
| jcsackett | bdmurray: i'm in the middle of running to somewhere that has better internet than my currently failing home spot. i'll check it as soon as i'm moved. | 15:15 |
| bdmurray | jcsackett: okay, thanks! | 15:16 |
| === al-maisan is now known as almaisan-away | ||
| jml | stub: https://code.launchpad.net/~jml/launchpad/db-rename-archive-commercial-to-suppress/+merge/109179 | 16:03 |
| stub | jml: The horrible hack to allow both the old and new column name has landed? | 16:06 |
| jml | stub: indeed it has. | 16:06 |
| jml | stub: and appears to work. | 16:06 |
| stub | I feel dirty | 16:06 |
| jml | stub: it's not deployed yet. | 16:06 |
| stub | r=stub, with a comment that the code needs to be deployed before the patch can go live. | 16:08 |
| stub | Easiest way to guarantee that is to not land it until after deployment. | 16:08 |
| jml | stub: that'll probably happen, since I need lifeless's patch before landing anyway, right? | 16:09 |
| stub | jml: No, you only need one of our approvals. | 16:09 |
| jml | stub: ah ok. I'll wait until after I hear about the deployment then. | 16:11 |
| stub | That would be FDT tomorrow I guess | 16:11 |
| cjwatson | NDT, wouldn't it be? | 16:12 |
| stub | er, yer | 16:14 |
| jml | sorry, I'm not familiar with those acronyms. | 16:14 |
| stub | fast downtime, no downtime | 16:14 |
| cjwatson | fastdowntime -> db deploy, nodowntime -> code deploy | 16:14 |
| jml | oh right. sorry. | 16:14 |
| jml | yeah, NDT | 16:15 |
| jml | stub: oh, the patch number ends with -0. Is that a problem? | 16:26 |
| stub | No problem. Numbers currently have no meaning besides ordering and uniqueness. | 16:26 |
| jml | stub: oh, the patch number ends with -0. Is that a problem? | 16:27 |
| jml | stub: ah ok. cool. | 16:27 |
| jcsackett | bdmurray: are you still around? took me a good bit longer to get setup in new location than i anticipated, but i've looked at your MP and have some comments/questions. | 16:30 |
| bdmurray | jcsackett: yes still here | 16:33 |
| jcsackett | bdmurray: awesome. so, your modification does change the sort order, but it has the side effect of putting any user that is subscribed via the "Subscribe someone else" button at the bottom of the list. I *think* this is probably why prepend was being used--so in long subscriber lists a user could still see the person they added was in fact added. | 16:34 |
| bdmurray | jcsackett: ah, how did you go about determining the side effect? | 16:35 |
| jcsackett | bdmurray: i just used the "subscribe someone else" button above the subscribers list. given the change was commented as "add user to start of list" i wondered if that wasn't intentional. | 16:36 |
| bdmurray | jcsackett: oh right, running it makes sense. Do you any suggestions are where to go from here? | 16:37 |
| jcsackett | bdmurray: i do. i think you can probably update addSubscriber to take an argument named, say "add_new" that tells addSubscriber whether it's adding something being loaded from a list, and should be appended, or is adding a brand new person from the UI, and should be prepended. | 16:39 |
| jcsackett | you could default that to 'false' and then just update the call that's bound to the "Subscribe someone else" button to set it to true. | 16:40 |
| bdmurray | jcsackett: okay, thanks I'll have a go at it in a bit | 16:40 |
| jcsackett | bdmurray: awesome. i'll update your MP with these notes. ping me if you run into any problems--i'm happy to help. :-) | 16:40 |
| === salgado is now known as salgado-lunch | ||
| jml | james_w: I've ec2 landed your branch | 17:21 |
| james_w | thanks | 17:21 |
| james_w | I hope it stays landed | 17:21 |
| === matsubara is now known as matsubara-afk | ||
| sinzui | jcsackett, do you have time to review https://code.launchpad.net/~sinzui/launchpad/unlock-sprite/+merge/109206 | 18:17 |
| jcsackett | sinzui: i just opened the MP, actually. :-) | 18:17 |
| sinzui | sorry about the svg | 18:17 |
| jcsackett | no worries. | 18:21 |
| jcsackett | sinzui: r=me. looks good, thanks. :-) | 18:23 |
| sinzui | fab | 18:23 |
| bdmurray | I forget what is username / password for launchpad.dev? | 18:24 |
| bdmurray | jcsackett: ^? | 18:32 |
| rick_h | bdmurray: you can use mark@example.com with test | 18:35 |
| bdmurray | jcsackett: I've updated my branch | 19:09 |
| * jcsackett looks | 19:11 | |
| jcsackett | bdmurray: r=me. do you have landing rights, or do you need an assist? | 19:22 |
| bdmurray | jcsackett: its been a long time, so I could use some handholding landing it. I mean I'd like to see if I still can. | 19:23 |
| jcsackett | bdmurray: dig. well, first off, are you set up for ec2 test/land? (https://dev.launchpad.net/EC2Test) | 19:29 |
| bdmurray | jcsackett: yes, I should be set what is the proper comamnd to test and land? | 19:44 |
| jcsackett | bdmurray: If lp-dev-utils is on your path you just use "ec2 land". | 19:47 |
| sinzui | bdmurray, that would be "ec2 land https://code.launchpad.net/~brian-murray/launchpad/bug-912137/+merge/109045" | 19:51 |
| sinzui | jcsackett, are you available for a pre-imp talk. I am trying to teach Lp that projects can go uncommercial | 19:53 |
| jcsackett | I am actually in a coffee shop since wifi died at home. I don't think I could hear you over background noise. | 19:53 |
| jcsackett | I am told wifi should be restored in about an hour? | 19:54 |
| jcsackett | Well, Internet, not wifi. | 19:54 |
| sinzui | okay | 19:54 |
| bdmurray | tests are faster now right? | 19:58 |
| sinzui | bdmurray, yui tests are, the suite itself takes 6 hours and must be run on ec2, the buildbot twice | 19:59 |
| sinzui | bdmurray, expect a day for it to arrive on qastaging | 19:59 |
| * sinzui looks at diff | 20:00 | |
| sinzui | bdmurray, I suggest an alternate plan since you fixed a js-only issue | 20:01 |
| sinzui | bdmurray, run ./bin/test -vvc --layer=YUI | 20:01 |
| sinzui | if that layer passes, we can submit your branch directly to PQM | 20:02 |
| bdmurray | sinzui: I'm not in a hurry and want to make sure my ec2 setup is good so I think I'll let it run | 20:03 |
| sinzui | okay | 20:03 |
| jcsackett | sinzui: i am home, and i appear to have full connectivity. do you still want to chat, or have you moved on? | 21:01 |
| sinzui | oh, fab, yes lets talk | 21:01 |
| jcsackett | sinzui: i've started a hangout. | 21:04 |
| sinzui | okay | 21:04 |
| === salgado is now known as salgado-afk | ||
| === jcsackett changed the topic of #launchpad-dev to: http://dev.launchpad.net/ | On call reviewer: - | Firefighting: - | Critical bugs: 3.47*10^2 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!