[00:36] hmm ended up ditching colander, validation isn't really complex enough to justify another dep, but worth looking at for larger services. [04:47] wgrant: token invalidation all sorted, was thinking we'd run the sweeper every minute? [04:57] would mean a token can potentially live 1m longer than it's ttl, although I would imagine most will be manually invalidated [05:48] blr: I don't think it's particularly necessary to run it that frequently. Presumably the thing validating a token would check the TTL before allowing it. [05:51] wgrant: it does, but what about the case where a token is manually invalidated and still within the ttl? [05:51] I suppose I should check both [05:53] Right, the pruning cronjob is just to clean up the DB. [05:53] oh, I am checking both :P [05:55] will add the cron job and schema migration to the charm tomorrow, then should be a in a reasonable state. [05:56] I seem to be leaking 'a's... time for coffee. [05:56] Heh [05:56] Sounds good [09:24] wgrant: cross-ownership deletion constraints: we have those on recipes too, right? [09:24] cjwatson: yes, and they're horrible. [09:24] These are marginally worse (as they involve archives too), but not substantially. [09:25] I'd never noticed them before. I assume it means that you can prevent somebody else from deleting their branch by creating a recipe for it [09:25] Right, exactly. [09:25] There are a couple of bugs about it. [09:25] How about making them nullable in this case? You wouldn't be able to issue any new builds, but the existing snap builds could stick around/. [09:26] Right, that might be reasonable. [09:26] It is useful to compare the FKs to simple URLs from the snapcraft.yaml [09:27] Only the root branch actually has the FK enforced. [09:27] Because the rest are in freeform text from Launchpad's PoV. [09:27] Freeform but parsed, but yes. [09:27] Not from Launchpad's PoV. [09:28] SourcePackageRecipeDataInstruction [09:28] Unless you're talking about snaps. [09:29] Oh, yeah, I mean snaps. [09:29] We can't enforce that references in snapcraft.yaml remain good. [09:29] So perhaps we need not enforce that the reference *to* snapcraft.yaml remains good. [09:29] Right, hence why staleness doesn't work. [09:30] One could argue that the branch/repo and the archive are just references that may become stale like any other. [09:30] In fact the git_path has to be. [09:30] Indeed. [09:31] I'll weaken the constraint. [09:32] It is similar to what we do with SPRB.recipe. [09:33] Weird place to break the link. [09:34] I definitely prefer having SnapBuild.snap non-nullable. [09:34] For Snaps it is simpler, as the SnapBuilds are likely themselves deletable. [09:34] It was not so with SourcePackageRecipeBuilds. [09:34] Well, it would have been possible, but important authorship information would also have been lost. [09:35] For packages in archives. [09:35] True [09:35] Also one of these days I shall remember that unique constraints are indexes. [09:35] Indeed they are. [09:37] cjwatson: Have you thought about how private branches/repos and snaps might work? [09:42] Not yet. And hm, I'm missing information_type on snap, aren't I. [09:42] I think that's the only piece that requires consideration at the DB level [09:42] It is conceivable. [09:42] Other than that it's mostly granting some kind of token to the builder [09:43] And I do need to handle processors as well; not just for restrictedness, but also just to configure which architectures to build for. [09:44] That is true, if they're not going to be explicitly requested. [09:44] I guess we don't have daily builds yet so it could be entirely a matter of explicit requests, but that seems like poor UX. [09:45] Would be nicer to record which arches are currently being built and then have a button to build everything now pls. [10:49] wgrant: Perhaps SnapArch would be a reasonable thing to have? I'm thinking it could pretty much exactly duplicate Archive.processors etc. [10:49] cjwatson: That's certainly what I'd do. [10:50] Maybe without the enabled_restricted_processors stuff. [10:51] Right, it doesn't particularly need the backward compat bits. [10:51] I mean, we want to avoid letting people set restricted processors, but it doesn't need that name. [10:51] Oh, right. [10:51] It may be a good opportunity to fix it for archives too. [10:53] I don't want to bite off any more just right now, my mouth is full already :P [10:54] Damn. [10:58] Is there any point in having SnapArch.id, given that it'd be UNIQUE (snap, processor) anyway? [11:01] No. [11:01] ArchiveArch probably dates from not long after the Storm transition. [11:11] wgrant: OK. Do my changes to https://code.launchpad.net/~cjwatson/launchpad/db-snappy/+merge/265332 look OK? [11:12] Updating the model code now [11:14] 5+public.snaparch = SELECT, UPDATE [11:14] orlyu [11:15] Hm yeah that's probably wrong [11:15] Nothing other than launchpad_main should need UPDATE [11:16] Nothing should ever be updating them, should it? [11:17] Fair point. Both fixed. [11:17] I think that's reasonable then. Thanks. [11:19] I'll add and test Snap.processors before landing. [11:28] Oh, information_type, I forgot that too. [12:27] wgrant: If you're still up, do you think I should have Snap.information_type (like proper shareable things) or just Snap.private (like Archive)? Maybe I should defer privacy handling until a bit later, or I'll never get this going [12:35] cjwatson: I would prefer Snap.private, but I have long had a potentially unreasonable vendetta against information_type, so take it with a grain of salt. [12:36] I see no benefit here; we'll be restricting it to two values anyway... [12:37] I guess if it's just private I can probably get that into the first pass. [12:37] The main difficulty with privacy is working out who can see it. [12:37] Since these things aren't within another privacy context (ignoring the team). [12:38] Right, that's why I was sort of inclined to defer to start with [12:38] I cannot disagree. [12:38] It is not significantly more difficult to retrofit. [12:38] And it's a bit multifaceted - do we hide the snap if the branch is invisible? [12:39] Oh it is terrible, yes. [12:39] Though I suppose that just goes on the security adapter [12:39] But still [12:39] We have the snap, the source, and the archive. [12:39] All of which may be differently private. [12:39] It is quite ghastly. [12:41] Anyway, I think I've addressed all your review comments now, so landing db-snappy [12:42] Let's see if I remembered everything this time [12:50] Oh, damnit, missing ON DELETE CASCADE [12:51] cjwatson: Where were you going to use that? [12:52] SnapArch.snap [12:52] Ah, I suppose that makes sense. [12:55] How do I add that after the fact? ALTER TABLE SnapArch DROP CONSTRAINT snaparch_arch_fkey; ALTER TABLE SnapArch ADD FOREIGN KEY (snap) REFERENCES snap ON DELETE CASCADE? [12:55] FKs aren't constraints. [12:55] Seems annoying not to be able to alter that in one step. [12:56] They kind of are. \d lists them as such. [12:56] Also no ALTER TABLE ... DROP FOREIGN KEY [12:56] Oh, that's right. They appear as constraints to some DML. [12:57] I'm not sure I'd bother, anyway. [12:57] There are other uncascadable FKs to Snap, so it needs a whole lot of custom deletion code anyawy. [12:59] I guess. Just noticed when I merged up as far as snap-builds and tests started failing. [13:00] Its custom deletion code currently only consists of checking that there are no builds and then store.remove [13:01] With that check there are no other uncascadable FKs. [13:02] No uncascadable FKs except the one that will affect anything that's actually been used, indeed :) [13:03] I suppose in future we might want to be able to delete those. It makes a bit more sense than it would for recipes where they'd have to cascade into package builds and obviously can't. [13:04] Right, exactly. [13:15] Snap.requestBuild is going to need some fixing up to honour processors, and maybe we want Snap.requestBuilds [20:39] morning