mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 45057: Made `unzip` overwrite existing files without prompting.
Date Sat, 19 Mar 2016 21:11:55 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45057/#review124422
-----------------------------------------------------------


Fix it, then Ship it!




Thanks for the patch! This looks great! I made some comments on the first test, but the same
comments should apply to other tests as well. Once the path is updated, I'll commit it. Thanks!


src/tests/fetcher_tests.cpp (line 642)
<https://reviews.apache.org/r/45057/#comment186968>

    2 lines apart



src/tests/fetcher_tests.cpp (lines 647 - 649)
<https://reviews.apache.org/r/45057/#comment186969>

    Can you create a temp file under os::getcwd()? The TemporaryDirectoryTest fixture will
cleanup the current working directory after the test finishes/failed.
    
    If the test fails, the existing code will leak the tmp zip file.



src/tests/fetcher_tests.cpp (line 648)
<https://reviews.apache.org/r/45057/#comment186972>

    Kill this line.



src/tests/fetcher_tests.cpp (lines 657 - 660)
<https://reviews.apache.org/r/45057/#comment186970>

    Indentation here should be 4:
    
    ```
    ASSERT_SOME(...
        "UES....."
        "...."
        "...").get()));
    ```



src/tests/fetcher_tests.cpp (line 661)
<https://reviews.apache.org/r/45057/#comment186971>

    Insert a new line above. We typically insert a new line after a multi-line statement.



src/tests/fetcher_tests.cpp (line 682)
<https://reviews.apache.org/r/45057/#comment186973>

    s/extract/extractedFile/
    
    Please use path::join(os::getcwd(), "hello");



src/tests/fetcher_tests.cpp (line 687)
<https://reviews.apache.org/r/45057/#comment186974>

    This is not necessary if you put the zip file under os::getcwd() as I mentioned above.



src/tests/fetcher_tests.cpp (line 689)
<https://reviews.apache.org/r/45057/#comment186975>

    2 lines apart please.



src/tests/fetcher_tests.cpp (line 694)
<https://reviews.apache.org/r/45057/#comment186976>

    Ditto. Kill this line.


- Jie Yu


On March 19, 2016, 10:26 a.m., Tomasz Janiszewski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45057/
> -----------------------------------------------------------
> 
> (Updated March 19, 2016, 10:26 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4885
>     https://issues.apache.org/jira/browse/MESOS-4885
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Made `unzip` overwrite existing files without prompting.
> 
> 
> Diffs
> -----
> 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45057/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>


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