[Freeipa-devel] Github review feature
Martin Basti
mbasti at redhat.com
Fri Sep 16 07:17:57 UTC 2016
Sorry for stealing your thread, but you started asking about github
review emails :)
Standard review inline comments are disabled on purpose, each comment
generates one email, so we decided that is better after review to write
a regular comment "NACK, please see inline comments" or so.
I would expecting that the new review feature is sending all comments in
one batch, but I was wrong. I used the new review feature (with the
pending comments) but when I sent it I received all comments as single
notifications again, so again one inline comment = one email
Even worse it is with states of review (approved, required change). I
didn't received any notification from github related to this (not sure
if is part of any inline comment message or just not implemented yet).
This is not documented in their API docs (according David) so we cannot
use it our tools yet.
Generally adding Labels ACK/Rejected are more visible and filters can be
made easily.
So for now I would stay with our old workflow and not extend email
notifications. We can play with new review feature for longer time and
decide if it is worth to use it (and change email notification accordingly)
Martin^2
On 15.09.2016 22:49, Ben Lipton wrote:
> On 09/15/2016 02:12 AM, jcholast wrote:
>> jcholast commented on a pull request
>>
>> """
>> In addition to my inline comments above:
>>
>> 1. "Certificate mapping" does not really evoke "certificate request templating" to me, and is also used in the context of mapping identities to certificates. Could we use a more suitable name to avoid confusion?
>> 2. The `ipalib.certmapping` module is used only in `ipaclient`, so that's where it should be located. It can be moved to `ipalib` later if necessary.
>> 3. I don't think `IPAExtension` deserves it's own module, at least not now.
>> """
>>
>> See the full comment athttps://github.com/freeipa/freeipa/pull/10#issuecomment-247244120
>>
>>
> Tried sending my comments as a "review" (new Github feature) and it
> seems they don't get sent to the list that way. So:
>
> Thanks for the comments! I've fixed the simple ones and replied to the
> rest. Regarding your comments about file organization:
>
> 1. I quite agree that certmapping isn't a good name for what this
> turned out to be. With the convention of naming modules after the
> objects they model, perhaps a good name would
> be|certrequest|or|csr|? The command could be renamed to something
> like|certrequest-get-data|(or|certrequest-get-script|).
> 2. Just to confirm, you're suggesting just moving these classes to
> the|ipaclient.plugins.<whatever we end up calling it>|module?
> 3. Seems reasonable, I've moved it into the ipalib module for now. It
> will go wherever the contents of that module end up.
>
> Logistical stuff:
>
> * Now that this is under review I won't add any more content. Are
> you ok with the two commits about testing being part of this
> review or should I remove them?
> * If you run rebase --autosquash with the latest commit it doesn't
> actually apply cleanly, but I'm trying not to change history while
> it's being reviewed, so I'll do the rebase later on if that's ok?
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160916/f2710d59/attachment.htm>
More information about the Freeipa-devel
mailing list