As discussed, the flag needs to move from the Product table to the Branch table to support future use cases.


(13:23:44) stub: thumper: You sure an enum for the code review approval policy is enough? I thought that might end up as a new table containing several booleans or enums to twiddle with.
(13:25:31) stub: The column name is going to need to change though ('review' is generic, and in in particular confusing with Product.reviewed and Product.reviewer_whiteboard already existing. Need to be explicit with code_revew_approval_policy I think)
13:30
14:45
(14:48:01) Tim Penhey (thumper): stub: there is no way I'm going to suggest that we support complexly defined review policies
(14:48:03) Tim Penhey (thumper): stub: I've defined the four most common (as I see it)
(14:48:14) Tim Penhey (thumper): stub: and they are selectable through an enum
(14:48:20) stub: ok
(14:48:23) Tim Penhey (thumper): stub: right now we only support products
(14:48:46) Tim Penhey (thumper): stub: but I'd like to support package sets when they exist
(14:49:29) Tim Penhey (thumper): stub: the only other idea is perhaps we may want to be able to define policies on a branch basis
(14:49:35) Tim Penhey (thumper): stub: which would allow us to set it for certain package branches
(14:49:47) Tim Penhey (thumper): stub: or to define different policies for different branches on a given product
(14:49:53) Tim Penhey (thumper): stub: what are your thoughts on this?
(14:50:16) Tim Penhey (thumper): stub: the reason I've not done it yet is that 99% of branches won't need to set one
(14:50:24) Tim Penhey (thumper): stub: so I'm loathed to put it on to the branch table
(14:50:47) stub: Would we enable it for non-trunk branches in the UI?
(14:51:01) Tim Penhey (thumper): we could
(14:51:09) Tim Penhey (thumper): right now I don't suggest that we do
(14:51:12) Tim Penhey (thumper): but...
(14:51:16) ***Tim Penhey (thumper) shrugs
(14:51:47) stub: It seems silly to even display if for development branches, which are sources not sinks in general.
(14:51:58) Tim Penhey (thumper): exactly
(14:52:13) Tim Penhey (thumper): another reason why I don't really want it on branches
(14:52:25) stub: Where will the flag go for package branches (assuming we don't shuffle it to the Branch table)?
(14:52:39) Tim Penhey (thumper): on the yet to be defined package sets
(14:53:52) stub: A package can be in multiple packagesets
(14:54:27) Tim Penhey (thumper): ah, poos
(14:54:29) stub: So I don't see how it fits there
(14:54:38) Tim Penhey (thumper): I'm not sure where to set it for package branches
(14:54:51) stub: Moving the flag to Branch might be the best approach.
(14:54:57) Tim Penhey (thumper): or even how often it'll be requested
(14:55:06) stub: Even if it is NULL for most Branches.
(14:55:31) Tim Penhey (thumper): hmm...
(14:56:02) stub: I suspect the code review process for packages will be YAGNI myself - are there big teams that coordinate to create packages?
(14:56:21) stub: Maybe kernel, and their processes are probably more complex than we can support in code review already.
(14:56:32) Tim Penhey (thumper): we could enable the UI for series branches
(14:57:20) Tim Penhey (thumper): or official package branches
(14:57:20) Tim Penhey (thumper): any user could tweak it with the API if they really want to
(14:57:20) Tim Penhey (thumper): I don't know
(14:57:20) Tim Penhey (thumper): but we don't know what will happen with the package branches on LP yet
(14:57:20) Tim Penhey (thumper): everything is speculation right now
(14:57:38) stub: Yup. Sticking it in Branch is the most flexible option, as it supports all the cases (even the maybe YAGNI ones)
(14:57:53) Tim Penhey (thumper): yeah, it isn't too much rework
(14:58:08) Tim Penhey (thumper): fooey
(14:58:10) stub: And then 'review' isn't ambiguous anymore (is it?)
(14:58:23) Tim Penhey (thumper): heh, no
(14:58:42) Tim Penhey (thumper): and it would make jml happy that we can have it working for package branches
(14:58:48) Tim Penhey (thumper): instead of them being second class citizens
(14:58:55) Tim Penhey (thumper): ok
(14:58:58) Tim Penhey (thumper): lets do that then
(14:59:00) Tim Penhey (thumper): a nullable column
(14:59:18) Tim Penhey (thumper): I can then use the NoPolicy to save NULL
(14:59:47) Tim Penhey (thumper): and give it a db value of 0
15:00
(15:00:03) Tim Penhey (thumper): I haven't done the UI yet anyway
(15:00:06) Matthew Revell (mrevell) [<email address hidden>] entered the room.
(15:00:11) Tim Penhey (thumper): only settable through the API right now
(15:00:27) Matthew Revell (mrevell): hi
(15:00:27) Tim Penhey (thumper): hi mrevell
(15:00:29) stub: We generally explicitly have an UNKNOWN or UNSET enum value rather than allowing NULLs.
(15:01:45) Julian Edwards (bigjools) [<email address hidden>] entered the room.
(15:01:45) Tim Penhey (thumper): well, we could keep the default of 0
(15:01:45) Tim Penhey (thumper): or 1
(15:01:45) Tim Penhey (thumper): which means no policy
(15:01:45) Tim Penhey (thumper): which is what I defined for products
(15:01:46) Tim Penhey (thumper): stub: you happier with that then?
(15:02:17) Julian Edwards (bigjools): morning dudes
(15:02:19) stub: thumper: Will a single enum be useful for storing the product policies and the package policies? Are they the same set of values or will the policies be different?
(15:02:21) Tim Penhey (thumper): hi bug
(15:02:25) Tim Penhey (thumper): d'oh
(15:02:38) Tim Penhey (thumper): bigjools: bug-tab didn't work
(15:02:50) stub: thumper: The integer value is irrelevant except for database ordering, and that only matters to the application so up to you.
(15:02:51) Julian Edwards (bigjools): hi tum :)
(15:03:23) Tim Penhey (thumper): stub: I'll rip it off the product and set it on the branch itself
(15:04:04) Tim Penhey (thumper): stub: unless you think we should have it on the product too
(15:04:17) Tim Penhey (thumper): stub: which would mean we'd need an UNSET as well as NO POLICY
(15:05:39) stub: Moving the enum from Product to Branch should be fine.
(15:06:38) Tim Penhey (thumper): ok, I think that'd work enough if we enable the UI for series branches
(15:06:49) Tim Penhey (thumper): so people could set them for development focus
(15:06:53) stub: Allowing NULL for branches that have no way of having the policy set and where the policy is just ignored anyway makes sense.
(15:06:57) Tim Penhey (thumper): or special ones to their heart's content
(15:09:02) stub: If UNSET (or NULL) is the same as NO POLICY though, we might as well just use NO POLICY unless we need to capture the fact the policy has never been explicitly set by the user or some automatic process.
(15:09:17) Tim Penhey (thumper): stub: well, any branch could set it through the api if they want to
(15:09:17) Tim Penhey (thumper): stub: I don't see why we should disallow it
(15:09:17) Tim Penhey (thumper): stub: just not through the UI for most branches
(15:09:43) Tim Penhey (thumper): right
(15:09:48) stub: So I'd go for a NOT NULL enum column with a DEFAULT for the default policy.
(15:09:52) Tim Penhey (thumper): stub: although a user could choose 'No policy'
(15:09:58) Tim Penhey (thumper): if they want to stop auto-approval
(15:10:38) Tim Penhey (thumper): stub: ok, so the agreement is to move the not null enum column to Branch from Product, yes?
(15:10:45) stub: Yes
(15:12:06) Tim Penhey (thumper): ok, could I get you to add a comment to that effect on the review so jml can see it?
(15:12:22) Tim Penhey (thumper): stub: you could just paste in the irc conversation if you like :)
(15:12:30) stub: ok