<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>Sorry for stealing your thread, but you started asking about
github review emails :)</p>
<p><br>
</p>
<p>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.</p>
<p>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</p>
<p>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.</p>
<p>Generally adding Labels ACK/Rejected are more visible and filters
can be made easily.</p>
<p><br>
</p>
<p>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)</p>
<p><br>
</p>
<p>Martin^2<br>
</p>
<br>
<div class="moz-cite-prefix">On 15.09.2016 22:49, Ben Lipton wrote:<br>
</div>
<blockquote
cite="mid:a6099b95-5b0c-000d-cb43-3e9de4a492d9@redhat.com"
type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
On 09/15/2016 02:12 AM, jcholast wrote:<br>
<blockquote
cite="mid:gh-freeipa%2Ffreeipa-10-2016-718d3b9d-8d77-4572-8430-f64bf0b1906f@freeipa-github-notification.redhat.com"
type="cite">
<pre wrap="">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 at <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://github.com/freeipa/freeipa/pull/10#issuecomment-247244120">https://github.com/freeipa/freeipa/pull/10#issuecomment-247244120</a>
</pre>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
Tried sending my comments as a "review" (new Github feature) and
it seems they don't get sent to the list that way. So:<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<p style="box-sizing: border-box; margin-top: 0px !important;
margin-bottom: 16px; color: rgb(51, 51, 51); font-family:
-apple-system, BlinkMacSystemFont, "Segoe UI", Roboto,
Helvetica, Arial, sans-serif, "Apple Color Emoji",
"Segoe UI Emoji", "Segoe UI Symbol";
font-size: 14px; font-style: normal; font-variant-ligatures:
normal; font-variant-caps: normal; font-weight: normal;
letter-spacing: normal; orphans: 2; text-align: start;
text-indent: 0px; text-transform: none; white-space: normal;
widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(255, 255, 255);">Thanks for the comments!
I've fixed the simple ones and replied to the rest. Regarding
your comments about file organization:</p>
<ol style="box-sizing: border-box; padding-left: 2em; margin-top:
0px; margin-bottom: 16px; color: rgb(51, 51, 51); font-family:
-apple-system, BlinkMacSystemFont, "Segoe UI", Roboto,
Helvetica, Arial, sans-serif, "Apple Color Emoji",
"Segoe UI Emoji", "Segoe UI Symbol";
font-size: 14px; font-style: normal; font-variant-ligatures:
normal; font-variant-caps: normal; font-weight: normal;
letter-spacing: normal; orphans: 2; text-align: start;
text-indent: 0px; text-transform: none; white-space: normal;
widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(255, 255, 255);">
<li style="box-sizing: border-box;">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<span
class="Apple-converted-space"> </span><code
style="box-sizing: border-box; font-family: Consolas,
"Liberation Mono", Menlo, Courier, monospace;
font-size: 11.9px; padding: 0.2em 0px; margin: 0px;
background-color: rgba(0, 0, 0, 0.0392157); border-radius:
3px;">certrequest</code><span class="Apple-converted-space"> </span>or<span
class="Apple-converted-space"> </span><code
style="box-sizing: border-box; font-family: Consolas,
"Liberation Mono", Menlo, Courier, monospace;
font-size: 11.9px; padding: 0.2em 0px; margin: 0px;
background-color: rgba(0, 0, 0, 0.0392157); border-radius:
3px;">csr</code>? The command could be renamed to something
like<span class="Apple-converted-space"> </span><code
style="box-sizing: border-box; font-family: Consolas,
"Liberation Mono", Menlo, Courier, monospace;
font-size: 11.9px; padding: 0.2em 0px; margin: 0px;
background-color: rgba(0, 0, 0, 0.0392157); border-radius:
3px;">certrequest-get-data</code><span
class="Apple-converted-space"> </span>(or<span
class="Apple-converted-space"> </span><code
style="box-sizing: border-box; font-family: Consolas,
"Liberation Mono", Menlo, Courier, monospace;
font-size: 11.9px; padding: 0.2em 0px; margin: 0px;
background-color: rgba(0, 0, 0, 0.0392157); border-radius:
3px;">certrequest-get-script</code>).</li>
<li style="box-sizing: border-box; margin-top: 0.25em;">Just to
confirm, you're suggesting just moving these classes to the<span
class="Apple-converted-space"> </span><code
style="box-sizing: border-box; font-family: Consolas,
"Liberation Mono", Menlo, Courier, monospace;
font-size: 11.9px; padding: 0.2em 0px; margin: 0px;
background-color: rgba(0, 0, 0, 0.0392157); border-radius:
3px;">ipaclient.plugins.<whatever we end up calling
it></code><span class="Apple-converted-space"> </span>module?</li>
<li style="box-sizing: border-box; margin-top: 0.25em;">Seems
reasonable, I've moved it into the ipalib module for now. It
will go wherever the contents of that module end up.</li>
</ol>
<p style="box-sizing: border-box; margin-top: 0px; margin-bottom:
16px; color: rgb(51, 51, 51); font-family: -apple-system,
BlinkMacSystemFont, "Segoe UI", Roboto, Helvetica,
Arial, sans-serif, "Apple Color Emoji", "Segoe UI
Emoji", "Segoe UI Symbol"; font-size: 14px;
font-style: normal; font-variant-ligatures: normal;
font-variant-caps: normal; font-weight: normal; letter-spacing:
normal; orphans: 2; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal; widows: 2;
word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(255, 255, 255);">Logistical stuff:</p>
<ul style="box-sizing: border-box; padding-left: 2em; margin-top:
0px; margin-bottom: 0px !important; color: rgb(51, 51, 51);
font-family: -apple-system, BlinkMacSystemFont, "Segoe
UI", Roboto, Helvetica, Arial, sans-serif, "Apple
Color Emoji", "Segoe UI Emoji", "Segoe UI
Symbol"; font-size: 14px; font-style: normal;
font-variant-ligatures: normal; font-variant-caps: normal;
font-weight: normal; letter-spacing: normal; orphans: 2;
text-align: start; text-indent: 0px; text-transform: none;
white-space: normal; widows: 2; word-spacing: 0px;
-webkit-text-stroke-width: 0px; background-color: rgb(255, 255,
255);">
<li style="box-sizing: border-box;">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?</li>
<li style="box-sizing: border-box; margin-top: 0.25em;">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?</li>
</ul>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
<br>
</body>
</html>