On Thu, Mar 8, 2018 at 12:46 PM, Jakub Hrozek <jhro...@redhat.com> wrote: > > >> On 8 Mar 2018, at 12:34, Pavel Březina <pbrez...@redhat.com> wrote: >> >> On 03/08/2018 12:22 PM, Jakub Hrozek wrote: >>>> On 8 Mar 2018, at 12:13, Fabiano Fidêncio <fiden...@redhat.com> wrote: >>>> >>>> On Thu, Mar 8, 2018 at 12:00 PM, Jakub Hrozek <jhro...@redhat.com> wrote: >>>>> >>>>> >>>>>> On 8 Mar 2018, at 10:33, Fabiano Fidêncio <fiden...@redhat.com> wrote: >>>>>> >>>>>> People, >>>>>> >>>>>> I've noticed that I'm getting a little bit lost with github and the >>>>>> way SSSD has its tags organized there. >>>>>> >>>>>> As it may actually affect other people (and in case it does not, let's >>>>>> just skip the following suggestion) ... I'd like to suggest the >>>>>> following tags to the project: >>>>>> >>>>>> - Accepted: We already have it; >>>>>> >>>>>> - Rejected: We already have it. >>>>>> >>>>>> - Tests needed: This one can either replace the "Changes Requested" >>>>>> (in case it's split in a few different tags) or be used together ... >>>>>> but the idea is to identify that tests are missing from a PR without >>>>>> going through the whole discussions happening there; >>>>> >>>>> What do you propose would be the action after tests needed? Should it be >>>>> a follow up PR, a ticket for the project, a ticket for downstream..? >>>>> >>>> >>>> After the "Tests needed" tag is added the developer should either: >>>> - Write the tests upstream (considering that we have infra for that, >>>> which is not the case for all the PRs) >>> Here I’m really worried that unless we have a ticket, this won’t happen. >>> Look at the “CI” milestone in pagure.. So I would say this case should >>> result in Changes requested, filing a ticket or asking downstream QE to >>> write a test. >> >> I would use this like this: >> >> a) You push a PR so it can be reviewed and you plan to provide tests later. >> >> b) You push a PR without tests and someone decides tests are mandatory for >> this PR. >> >>>> - Provide a "link" of the related downstream tests that were >>>> broken/were added passing >>>> >>> This makes sense, although I would argue this should already be default. >>> But if you don’t think so, we can try the tag and see how it goes. >>>> So, summing up, no ticket for the project, no ticket downstream ... >>>> just making clear that the PR is stalled because "Tests are needed". >>>> Does that make sense? >>>> >>>>> My worry about not supplying tests along with PRs is that the tests will >>>>> never be supplied..at least not in upstream.. >>>> >>>> I understand why you're worried and I agree with that. See the answer >>>> above and let me know if it fits your expectations. >>>> >>>>> >>>>>> >>>>>> - Depends on (or something similar): This one can either replace the >>>>>> "Changes Requested" (in case it's split in a few different tags) or be >>>>>> used together ... but the idea is to identify that we depend on >>>>>> somework that still has to be done (either another PR, ticket or >>>>>> something else that has to be implemented). Mind that I'm not sure >>>>>> whether we'd be able to simply add a field saying what the PR depends >>>>>> on … >>>>> >>>>> I think this makes sense. At least for a casual observer it would be >>>>> clear that there is no work needed on this P >>> >>>>>> >>>>>> - Postponed/Deferred: We have something similar for 2.0, but would be >>>>>> nice to have a way to clearly see in which release we're going to take >>>>>> a look into a specific PR without having to dig in the discussions. >>>>>> Here we could also have 1.16.1, 1.16.2, 2.0, … >>>>> >>>>> Tags are cheap, we can even have a postponed/$version. I guess even >>>>> depends/$PR might be OK as long as we only had a handful of dependecies. >> >> Patch dependencies are not that common so we can just set tag >> "depend/blocked" and write PR/ticket it depends on to the PR message as we >> do now. >> >>>>>> >>>>>> - Reworked: Although just removing the "Changes Requested" label is >>>>>> fine, maybe having a tag explicitly saying that something was Reworked >>>>>> would be a clean way to differentiate between new PRs and PRs which >>>>>> have been through a review already … >>>>> >>>>> I don’t know how this tag would be used, could you give me an example, >>>>> please? >>>> >>>> I usually have no idea (just by a quick look on github) whether a PR >>>> has been re-worked or it's a new PR that's never been reviewed. >>>> My impression is that having the "Reworked" tag would make simpler for >>>> people to jump in and do a follow-up review on what has been addressed >>>> in the first round(s) of review and then give their ACK instead of >>>> just leaving it for the reviewer. Of course, the same can be achieved >>>> without that tag ... so, it's just something that looks more >>>> "organized" to me. >>> OK, if this is something that was hitting you, maybe the tag might make >>> sense. But, then do you volunteer to maintain these tags? Because since I >>> didn’t see this as a problem, I’m afraid at least I wouldn’t maintain the >>> tags. >> >> The problem is that github does not sent any notifications that new patches >> were pushed and it does not show in the PR list. So we can use this tag to >> notify reviewers that new patches are pushed, but also please always write >> comment there since I don't usually go through webui but only through >> notification mails. > > You get the “synchronized” notification on sssd-devel. I’m not sure if you do > directly from github. My personal workflow was to check the PR dashboard for > anything without a tag and consider that as needing review. > > But again, I don’t want to impose my workflow for others, if others think > this tag makes sense, let’s try using it. > > (btw what about the github workflow document? Should we amend it after some > time of using the new tags?)
Yep, that would be the way to go. > >> >>>>>> Does the suggestion make sense? In case we have an agreement about >>>>>> this topic, may I re-tag our PRs and start using those new tags from >>>>>> new PRs? >>>>> >>>>> Another tag I was thinking of was “passes downstream tests”. With the >>>>> amount of time our downstream tests take, I’m not even sure how to >>>>> integrate them with the usual github flow like travis or centos CI use. >>>>> So I was thinking about a bot that would nightly scan PRs that have >>>>> neither “pass” or “fail” tag, bundle those up in an RPM, run the nightly >>>>> tests and report back using a tag. >>>> >>>> I really like the idea! >>>> >>>> Another tag that may be added is something like "Urgent" for PRs that >>>> are *really* *needed* for some specific reason (downstream, release, >>>> etc …) >>> Umm, fine, but how would others find out the list of urgent PRs? Isn’t it >>> then easier to drop a mail to the list? >> >> I would not overcomplicate it. Urgent PRs -> irc ping. >> _______________________________________________ >> sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org >> To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org > _______________________________________________ > sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org > To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org _______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
