mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 72079: Added workaround for Docker repositories not providing scope/service.
Date Tue, 04 Feb 2020 09:15:05 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message