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

Reply via email to