Merge filters #39

sbesson · 2012-12-21T14:12:01Z

This PR enhances the include/exclude filters used in the Merge command by allowing key:value filters to be passed. Three keys are currently defined: label, pr, user. pr filters are not propagated across submodules.

This allows to use the following syntax

scc merge develop --include user:ext_user pr:24 --exclude broken pr:45 user:session

Couple of questions:

currently the first condition for inclusion is a test that the PR owner belongs to the organisation which is almost always true. To be able to merge isolated PRs e.g. by number, we need to modify/disable this inclusion condition.
Josh suggested not to pass filters as series of arguments but instead individually. In this case, it would be nice to define a one character long option for include/exclude e.g.

scc merge develop -Iuser:ext_user -Ipr:24 -Ebroken -Epr:45 -Euser:session

do we want granularity for the order of precedence? currently PRs are first tested for inclusion using all filters then for exclusion using all filters

/cc @manics for discussion

The include and exclude filter are now parsed and stored in a dictionary with 3 keys: label (default), pr & user. Each filter can be attributed to the corresponding key using the key:value syntax in the command line.

For each filter (include or exclude), labels, user and pull request number are successively tested against the values of the filter dictionary. If the intersection is not null, it is logged in DEBUG.

joshmoore · 2012-12-21T16:10:27Z

@sbesson: see b62f51ed6777661ea328d for the beginnings of unit tests.

manics · 2012-12-21T17:15:00Z

currently the first condition for inclusion is a test that the PR owner belongs to the organisation which is almost always true. To be able to merge isolated PRs e.g. by number, we need to modify/disable this inclusion condition.

If a specific PR is given I think it's reasonable to automatically add the user, though we're not exactly snowed under by external PRs so a warning message would also do.

do we want granularity for the order of precedence? currently PRs are first tested for inclusion using all filters then for exclusion using all filters

Granularity would be nice but I don't think it's necessary for now, especially if the defaults are reasonable, e.g. scc merge develop --include pr:24 only includes PR24, scc merge develop --exclude pr:24 includes everything except PR24.... but that leaves the problem of whether external PRs should be automatically included.

sbesson · 2012-12-21T18:08:16Z

On 21 Dec 2012, at 17:15, Simon Li wrote:

currently the first condition for inclusion is a test that the PR owner belongs to the organisation which is almost always true. To be able to merge isolated PRs e.g. by number, we need to modify/disable this inclusion condition.

If a specific PR is given I think it's reasonable to automatically add the user, though we're not exactly snowed under by external PRs so a warning message would also do.

Agreed and that's how the current set of filters should be working. I'll work on test cases for this scenario.
do we want granularity for the order of precedence? currently PRs are first tested for inclusion using all filters then for exclusion using all filters

Granularity would be nice but I don't think it's necessary for now, especially if the defaults are reasonable, e.g. scc merge develop --include pr:24 only includes PR24, scc merge develop --exclude pr:24 includes everything except PR24.... but that leaves the problem of whether external PRs should be automatically included.

So this is the default behaviour that I'd like us to agree about.
Currently, on ome.git
scc merge develop

will merge any PR opened against develop by a member of openmicroscopy.

Having

 scc merge develop --include pr:24

merge only PR 24 would imply changing this default behaviour to merging no PR (for consistency else specifying an include filter would effectively decrease the number of merged PRs).
I do not mind implementing this change at all if we all agree but we need to take into account the impact on all the jobs using scc merge.

—
Reply to this email directly or view it on GitHub.

@sbesson

With @sbesson's changes, it now should be fairly straight-forward to write mocked tests. This is currently done using mox. Like Sandbox.py, Mocks.py provides a base class which should be used by all mocked tests.

These overloaded functions (for mox testins) do break the instantiation of real repositories.

Example of filter that was not working properly with lstrip(): "user:sbesson". In this case, s.lstrip(key + ":") will also strip the first "s" character.

Define 4 default modes: none, mine, org and alls. Allow better granularity and potentially any possibility when combined with include/exclude filters.

sbesson · 2013-01-02T20:17:53Z

@manics, @joshmoore: the last commit should add an optional parameter to control the default PRs to include.
Merge filters tests can be performed with the --info mode toggled on.

Thus,
scc.py merge --info --default mine --include user:joshmoore
will list all my PRs + all of Josh's

while
scc.py merge --info --default none --include pr:39
will only list PR 39.

Default mode is currently set to org (only PRs opened by public members of the organisation) for consistency with existing behavior until now.

sbesson · 2013-01-03T10:01:46Z

@joshmoore: the last commit should allow to renable the mox tests without breaking existing behaviour. Can be tested with:

python test/TestMerge.py

manics · 2013-01-03T10:50:33Z

@sbesson +1 --default is a nice way of handling things.

sbesson · 2013-01-08T18:04:21Z

@joshmoore: fixed the Travis build

joshmoore · 2013-01-09T08:54:23Z

Looks quite nice. I can imagine us wanted to add more defaults, but I can't think of any off-hand. ("followers", etc.) And eventually having a shortcut (e.g. -D) would probably be worth it, since using just the defaults might suffice in many cases. (In fact, one of my next TODOs is a way to update the submodules daily. I think scc merge -Dnone should suffice, but I need to test it.)

Otherwise, if this has been tested in a job, I think we should get it merged and configure it across the board.

Improve INFO log message to specify default mode

sbesson · 2013-01-09T11:50:54Z

@joshmoore: added the -D shortcut.
See https://github.com/snoopycrimecop/openmicroscopy/tree/snoopy/test for the latest push of SCC-merge

joshmoore · 2013-01-09T13:05:35Z

The one last thing ( 😇 ) I remember us discussing is whether to have "-E a b c" or "-E a -E b -E c". Since that's a fairly substantial departure, I'd say let's make a choice and stick with it (and I'll promise to never bring it up again, etc.)

manics · 2013-01-09T13:29:59Z

scc.py

+        self.filters["base"] = args.base
+        self.filters["default"] = args.default
+        if args.default == "org":
+            default_user = "any public member of the organgization"


Typo: organgization

Command for including/excluding multiple filters should be now formatted as: scc merge master -Dnone -Iuser:user1 -Iuser:user2 -Itest -Ilabel:test2 -Ipr:42 -Ipr:23 -Epr:39

manics · 2013-01-09T13:52:56Z

The one last thing ( 😇 ) I remember us discussing is whether to have "-E a b c" or "-E a -E b -E c". Since that's a fairly substantial departure, I'd say let's make a choice and stick with it (and I'll promise to never bring it up again, etc.)

Are you referring to:

do we want granularity for the order of precedence? currently PRs are first tested for inclusion using all filters then for exclusion using all filters

Individual arguments means you can mix up the order of -Is and -Es implying ordering matters (rightmost is the final determinant?), whereas only allowing one -I and/or one -E argument doesn't, and there should be a warning if multiple are passed.

joshmoore · 2013-01-09T14:13:48Z

There definitely shouldn't be a warning if multiple are passed, that's the point (as I understand it). But I can see how having -I a -E a -I b implies something about the ordering. Similar to -I for gcc, I think having them individual is sensible enough, but we can certainly mention something about the ordering being undefined (or similar).

manics · 2013-01-09T14:21:28Z

I meant there should be a warning if we stayed with the -I a b c case.

sbesson · 2013-01-09T14:30:07Z

After reflexion, I don't think we should be too concerned about my comment above until we reach a concrete situation where having the filters applied in the input order is critical. That being said, the current selection order (1- default, 2- include, 3- exclude) with the extra filtering options (label, user, pr number, default types) of this PR should already give us a lot of flexibility.

The last 2 commits implemented the switch to -I a -I b -Ic. See SCC-merge#100 for the application of these changes.

joshmoore · 2013-01-11T09:55:53Z

Merging for now. @manics, if in practice any of the choices causes a problem, another PR will certainly be called for.

Merge filters

sbesson added 5 commits December 21, 2012 12:04

Improve INFO log when using the --info optional argument

8d00370

Move filter parsing to a standalone method in Merge command

d4d2943

The include and exclude filter are now parsed and stored in a dictionary with 3 keys: label (default), pr & user. Each filter can be attributed to the corresponding key using the key:value syntax in the command line.

Modify filter check logic to account for new filter types

cc648dd

For each filter (include or exclude), labels, user and pull request number are successively tested against the values of the filter dictionary. If the intersection is not null, it is logged in DEBUG.

Add description of the new filter keys in the parser help

2170c39

Do not propagate filters PR numbers across submodules

d597635

joshmoore and others added 7 commits January 2, 2013 13:14

Use mox for writing mocked tests of Merge

f25b571

With @sbesson's changes, it now should be fairly straight-forward to write mocked tests. This is currently done using mox. Like Sandbox.py, Mocks.py provides a base class which should be used by all mocked tests.

Add missing test/Mock.py

3df551c

Pass filters directly to run_filter() and add action optional argument

054e653

Add filtering unit tests

d196202

Remove get_user() get_organization() function overloading

a7b3a81

These overloaded functions (for mox testins) do break the instantiation of real repositories.

Replace lstrip by replace to fix bug

1b37f2d

Example of filter that was not working properly with lstrip(): "user:sbesson". In this case, s.lstrip(key + ":") will also strip the first "s" character.

Add default optional argument to control the whitelisting mode

b86274c

Define 4 default modes: none, mine, org and alls. Allow better granularity and potentially any possibility when combined with include/exclude filters.

joshmoore mentioned this pull request Jan 3, 2013

"List my PRs" command #42

Closed

Re-overload get_user() and get_organization() method for mox tests

17f19fd

Rename dev_requirement to let the Travis build pass

6bf1a18

Add -D optional argument to specify default mode

5a19e62

Improve INFO log message to specify default mode

manics reviewed Jan 9, 2013
View reviewed changes

sbesson added 3 commits January 9, 2013 13:33

Add default mode and use single character options in commit message

ffda1f0

Force include/exclude filter arguments to be specified multiple times

084ffec

Command for including/excluding multiple filters should be now formatted as: scc merge master -Dnone -Iuser:user1 -Iuser:user2 -Itest -Ilabel:test2 -Ipr:42 -Ipr:23 -Epr:39

Fix commit id to be consistent with recent optional arguments changes

bc298ba

sbesson mentioned this pull request Jan 10, 2013

Continuous integration ome/omero-documentation#196

Merged

joshmoore added a commit that referenced this pull request Jan 11, 2013

Merge pull request #39 from sbesson/merge_filters

f1f0044

Merge filters

joshmoore merged commit f1f0044 into ome:master Jan 11, 2013

sbesson deleted the merge_filters branch January 11, 2013 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge filters #39

Merge filters #39

sbesson commented Dec 21, 2012

joshmoore commented Dec 21, 2012

manics commented Dec 21, 2012

sbesson commented Dec 21, 2012

sbesson commented Jan 2, 2013

sbesson commented Jan 3, 2013

manics commented Jan 3, 2013

sbesson commented Jan 8, 2013

joshmoore commented Jan 9, 2013

sbesson commented Jan 9, 2013

joshmoore commented Jan 9, 2013

manics Jan 9, 2013

manics commented Jan 9, 2013

joshmoore commented Jan 9, 2013

manics commented Jan 9, 2013

sbesson commented Jan 9, 2013

joshmoore commented Jan 11, 2013

Merge filters #39

Merge filters #39

Conversation

sbesson commented Dec 21, 2012

joshmoore commented Dec 21, 2012

manics commented Dec 21, 2012

sbesson commented Dec 21, 2012

sbesson commented Jan 2, 2013

sbesson commented Jan 3, 2013

manics commented Jan 3, 2013

sbesson commented Jan 8, 2013

joshmoore commented Jan 9, 2013

sbesson commented Jan 9, 2013

joshmoore commented Jan 9, 2013

manics Jan 9, 2013

Choose a reason for hiding this comment

manics commented Jan 9, 2013

joshmoore commented Jan 9, 2013

manics commented Jan 9, 2013

sbesson commented Jan 9, 2013

joshmoore commented Jan 11, 2013