Discussion:
[Freeipa-devel] [freeipa PR#616][opened] Simplify KRA transport cert cache
tiran
2017-03-17 09:49:02 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Author: tiran
Title: #616: Simplify KRA transport cert cache
Action: opened

PR body:
"""
In-memory cache causes problem in forking servers. A file based cache is
good enough. It's easier to understand and avoids performance regression
and synchronization issues when cert becomes out-of-date.

Signed-off-by: Christian Heimes <***@redhat.com>
"""

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/616/head:pr616
git checkout pr616
tiran
2017-03-17 09:49:17 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

tiran commented:
"""
Needs to be merged into ipa-4.5 branch, too.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/616#issuecomment-287311164
HonzaCholasta
2017-03-17 09:53:46 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

HonzaCholasta commented:
"""
NACK on the completely unnecessary changes in `_TransportCertCache` interface, variable names and formatting. Otherwise LGTM.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/616#issuecomment-287312193
MartinBasti
2017-03-17 09:54:00 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

MartinBasti commented:
"""
Please open backport ticket and put it into commit messsage
"""

See the full comment at https://github.com/freeipa/freeipa/pull/616#issuecomment-287312239
MartinBasti
2017-03-17 09:54:31 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

MartinBasti commented:
"""
Please open backport ticket and put it into commit messsage
"""

See the full comment at https://github.com/freeipa/freeipa/pull/616#issuecomment-287312239
tiran
2017-03-17 10:08:01 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

tiran commented:
"""
@HonzaCholasta I don't agree with you. Mutable mapping is too complex for a simple cache. My approach is KISS.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/616#issuecomment-287315292
tiran
2017-03-17 10:10:32 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Author: tiran
Title: #616: Simplify KRA transport cert cache
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/616/head:pr616
git checkout pr616
tiran
2017-03-17 12:24:44 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Author: tiran
Title: #616: Simplify KRA transport cert cache
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/616/head:pr616
git checkout pr616
HonzaCholasta
2017-03-20 06:20:50 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

HonzaCholasta commented:
"""
@tiran,
* patches targeted at backport branches should be as small as possible,
* it's hard to see what is an actual change and what is just a formatting / naming change,
* the formatting changes do not add any value, the code is already PEP8 compliant,
* using a mapping interface is KISS too.

I don't think this needs to be discussed further, either do the requested changes or this PR won't be merged.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/616#issuecomment-287685144
tiran
2017-03-20 09:27:53 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

tiran commented:
"""
Size of a patch is a wrong metric. It's about code complexity. My patch reduces code complexity and logic complexity. It also fixes at least two bugs: multi-process concurrency bug and logging bug that prevents the code from working correctly.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/616#issuecomment-287709913
HonzaCholasta
2017-03-20 11:59:06 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

HonzaCholasta commented:
"""
@tiran, you are right about the interface change, I was seeing things that are not there, I'm sorry. Please address inline comments (mainly the one about missing info in commit message, others are mostly nitpicks) and it's an ACK.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/616#issuecomment-287739826
tiran
2017-03-20 12:57:15 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Author: tiran
Title: #616: Simplify KRA transport cert cache
Action: edited

Changed field: body
Original value:
"""
In-memory cache causes problem in forking servers. A file based cache is
good enough. It's easier to understand and avoids performance regression
and synchronization issues when cert becomes out-of-date.

Signed-off-by: Christian Heimes <***@redhat.com>
"""
tiran
2017-03-22 10:19:20 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

Label: +rejected
tiran
2017-03-22 10:19:23 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Author: tiran
Title: #616: Simplify KRA transport cert cache
Action: closed

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/616/head:pr616
git checkout pr616
HonzaCholasta
2017-03-27 08:44:41 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Author: tiran
Title: #616: Simplify KRA transport cert cache
Action: reopened

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/616/head:pr616
git checkout pr616
HonzaCholasta
2017-03-27 08:44:44 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

HonzaCholasta commented:
"""
I guess you must have missed my last comment about the PR being almost OK - reopening.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/616#issuecomment-289390708
HonzaCholasta
2017-03-27 08:44:50 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

Label: -rejected
tiran
2017-03-27 08:49:08 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

tiran commented:
"""
I did not miss https://github.com/freeipa/freeipa/pull/616#issuecomment-287739826
"""

See the full comment at https://github.com/freeipa/freeipa/pull/616#issuecomment-289391731
HonzaCholasta
2017-03-28 08:11:35 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

Label: +pushed
HonzaCholasta
2017-03-28 08:07:59 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

Label: +ack
HonzaCholasta
2017-03-28 08:12:59 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Title: #616: Simplify KRA transport cert cache

HonzaCholasta commented:
"""
master:

* abefb64bea8ea1b8487ad87716e4a335555d19dc Simplify KRA transport cert cache

ipa-4-5:

* 2723b5fa5edc75901c8fbaf110a37c87df0aec87 Simplify KRA transport cert cache

"""

See the full comment at https://github.com/freeipa/freeipa/pull/616#issuecomment-289696220
HonzaCholasta
2017-03-28 08:13:03 UTC
Permalink
URL: https://github.com/freeipa/freeipa/pull/616
Author: tiran
Title: #616: Simplify KRA transport cert cache
Action: closed

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/616/head:pr616
git checkout pr616

Loading...