Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

labelling merge build #214

Open
atarkowska opened this issue Nov 30, 2016 · 5 comments
Open

labelling merge build #214

atarkowska opened this issue Nov 30, 2016 · 5 comments
Labels
enhancement New feature or request

Comments

@atarkowska
Copy link
Member

atarkowska commented Nov 30, 2016

It would be very handy to optionally narrow down how scc handles labels to merge PRs. At the moment filters based on union, would be very useful to apply intersection. This would be useful in a case where someone would like to test PR in isolation (read devspace).

Initial discussion:

  • -Iuser:#org -Elabel:!xyz (suggested)
  • -I label:xyz (AND|OR) -I label:xyz (more complicated)
@joshmoore
Copy link
Member

Also mentioned:

  • -Eunlabelled ("nolabels")?

@sbesson
Copy link
Member

sbesson commented Nov 30, 2016

At the moment, the contract of the merge command is to select Pull Requests/branches if they match any of the specified include filter and do no match any of the specified excluded filter i.e. it's actually a combination of OR and AND logic as in

-I user:#org -Ilabel:include -Elabel:exclude -Elabel:breaking

means

(user in org || label == include) && !(label == exclude || label == breaking)

The implementation is done in https://github.com/openmicroscopy/snoopycrimecop/blob/v0.6.7/scc/git.py#L857 for the accepted filter types (user, label and pr).

Being able to switch to combine filters using AND unconditionally is something that could be investigated but I am still unclear on how one would specify it at the command line and the impact on all combinations. Note some of our current workflows actively rely on the selection contract above e.g. to include both PRs from the organization and external PRs included via label.

The other option means no modification of the filtering contract but rather add new semantics to the filter values to define:

  • negations e.g. -I user:#org -Elabel:!xyz interpreted as (user in org) && !(label != xyz)
  • and/or special values e.g. -I user:#org -Elabel:none interpreted as (user in org) && !(label is none)

With regard to implementation the latter proposal would involve extending the run_filter method https://github.com/openmicroscopy/snoopycrimecop/blob/master/scc/git.py#L806 which contains the relevant logic.

@manics
Copy link
Member

manics commented Dec 5, 2016

If you kept the current semantics and syntax but added a switch to choose whether include/exclude has priority like the Apache 2.2 Order directive would that work)?

@sbesson
Copy link
Member

sbesson commented Dec 8, 2016

@aleksandra-tarkowska did some more testing and it looks like

scc merge -Dnone -Ilabel:<label>

would meet most of her requirements. The only remaining issue is that the "label via comment" functionality re-uses the user filter to whitelist comments. This means with the command above, no comment can be used as a label.

I see two options:

@atarkowska
Copy link
Member Author

atarkowska commented Jan 11, 2017

I think the above doesnt respect --breaking/exclude

So here is how I would use it in a CI:

  • merge only PRs that match development topic by adding --feature1
    • allow to choose if PRs should be included or not in the main merge build on ci master, examples:
      • Bob opened a PR against develop. These PRs should be tested only by the devspace in isolation but all together) very important for DB breaking changes, as some devspaces are running persistent DB, where restoring from backup is not an option
      • 2 developers works on the same component and PRs may conflict or affect testing individual features. Both should be tested in a different devspaces and don't be merged in a ci master.

As we go toward devspace, keep in mind that above would need to be applied in CI master in a first place, to make sure labelled PRs are not included.

As we are all familiar with the usage of exclude and breaking I think these flags could be respected as well, so even developing feature you can run 2 devspaces one (breaking and merge separately). We could argue that someone can just add --feature-breaking and that's it :)

Please correct if I am missing anything. If you think it is possible, could you please provide example. I am happy to test.

@sbesson sbesson mentioned this issue Jan 20, 2017
@sbesson sbesson added the enhancement New feature or request label Feb 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants