----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72079/#review219481 -----------------------------------------------------------
src/uri/fetchers/docker.cpp Lines 644-646 (patched) <https://reviews.apache.org/r/72079/#comment307647> This static method should be moved to the beginning of this file. src/uri/fetchers/docker.cpp Line 751 (original), 757 (patched) <https://reviews.apache.org/r/72079/#comment307648> Do we really this local variable? src/uri/fetchers/docker.cpp Line 969 (original), 976 (patched) <https://reviews.apache.org/r/72079/#comment307649> Ditto. src/uri/fetchers/docker.cpp Lines 1081-1082 (patched) <https://reviews.apache.org/r/72079/#comment307650> Should this be a static method? src/uri/fetchers/docker.cpp Lines 1081-1083 (patched) <https://reviews.apache.org/r/72079/#comment307651> Since this method will be called with both initial response and root response, I would suggest to just name its parameter as `response` rather than `initialResponse`. src/uri/fetchers/docker.cpp Lines 1122 (patched) <https://reviews.apache.org/r/72079/#comment307659> If the error is from the line 1089, then here the warning message will be something like: ``` Failed to get WWW-Authenticate header: <detailed error message> from <initialUri> ``` This is a bit odd, it's better to be something like: ``` Failed to get WWW-Authenticate header from <initialUri>: <detailed error message> ``` So maybe we should pass the `initialUri` into `getBearerAuthParam` as another parameter and include it in the error message there? src/uri/fetchers/docker.cpp Lines 1145 (patched) <https://reviews.apache.org/r/72079/#comment307652> A space between `>` and `{`. src/uri/fetchers/docker.cpp Lines 1183 (patched) <https://reviews.apache.org/r/72079/#comment307653> Too many spaces for the indent, 4 spaces should be enough. src/uri/fetchers/docker.cpp Line 1140 (original), 1186-1187 (patched) <https://reviews.apache.org/r/72079/#comment307654> Better to merge these two lines into a single line. src/uri/fetchers/docker.cpp Line 1142 (original), 1189 (patched) <https://reviews.apache.org/r/72079/#comment307656> Too many spaces for the indent, we need two less. src/uri/fetchers/docker.cpp Lines 1144-1145 (original), 1191-1194 (patched) <https://reviews.apache.org/r/72079/#comment307657> Why changing these from two lines to four lines? src/uri/fetchers/docker.cpp Line 1152 (original), 1201-1202 (patched) <https://reviews.apache.org/r/72079/#comment307658> Ditto. - Qian Zhang On Feb. 4, 2020, 1:54 a.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72079/ > ----------------------------------------------------------- > > (Updated Feb. 4, 2020, 1:54 a.m.) > > > Review request for mesos, Andrei Budnik and Qian Zhang. > > > Bugs: MESOS-10092 > https://issues.apache.org/jira/browse/MESOS-10092 > > > Repository: mesos > > > Description > ------- > > This patch adds a fallback Docker authorization server URI generation > mechanism (see MESOS-10092) for repository servers that provide no > "scope"/"service" params in the "WWW-Authenticate" header of the initial > "401 Unathorized" response. > > > Diffs > ----- > > src/uri/fetchers/docker.cpp 75df80bfd59323e06bf563d15a98af244b5b1874 > > > Diff: https://reviews.apache.org/r/72079/diff/1/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
