mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 49598: Added ability to parse docker v1 'ImageManifest' from 'docker inspect'.
Date Wed, 06 Jul 2016 21:02:22 GMT

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


Fix it, then Ship it!




Looks good, just some minor comments below.


src/docker/spec.cpp (lines 212 - 232)
<https://reviews.apache.org/r/49598/#comment206457>

    I was a bit surprised to see that the `"config"` field's keys use CamelCase. Maybe a more
explicit note, or a parentetical example of `"config"` fields being CamelCase?
    
    Seems we should more explicitly spell out to the reader that we **intentionally** do not
recurse when doing the transformation due to this? The only hint right now seems to be from:
    
    "it represents all of its **top level** keys in CamelCase." (emphasis mine)
    
    Which only tells me that we don't have to care about recursing, but it doesn't tell me
that we **must not** recurse (because e.g. "config" uses CamelCase).



src/docker/spec.cpp (lines 239 - 257)
<https://reviews.apache.org/r/49598/#comment206458>

    How about reducing the levels of nesting to make this a bit easier to read?
    
    ```
      foreachpair (const string& key, const JSON::Value& value, _json.values) {
        if (key == "Size") {
          continue; // "Size" is the only exception!
        }
        
        for (unsigned int i = 0; i < key.length(); ++i) {
          ...
        }
        ...
      }
    ```



src/docker/spec.cpp (lines 242 - 251)
<https://reviews.apache.org/r/49598/#comment206460>

    Using a foreach seems a bit simpler?
    
    ```
    foreach (char c, camelKey) {
      if (std::isupper(c)) {
        if (!snakeKey.empty()) {
          snakeKey += "_";
        }
        snakeKey += std::tolower(c);
      } else {
        snakeKey += c;
      }
    }
    ```
    
    More explicitly naming them camelKey and snakeKey seems a bit easier on the reader?



src/tests/containerizer/docker_tests.cpp (lines 672 - 673)
<https://reviews.apache.org/r/49598/#comment206461>

    Shouldn't this be an INTERNET test? (Do you also need CURL?)



src/tests/containerizer/docker_tests.cpp (lines 736 - 739)
<https://reviews.apache.org/r/49598/#comment206463>

    Thanks for the comment. Can you also add a TODO here to do a complete comparision?



src/tests/containerizer/docker_tests.cpp (lines 741 - 743)
<https://reviews.apache.org/r/49598/#comment206462>

    EXPECT_EQ?


- Benjamin Mahler


On July 4, 2016, 5:58 p.m., Kevin Klues wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49598/
> -----------------------------------------------------------
> 
> (Updated July 4, 2016, 5:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Yubo Li.
> 
> 
> Bugs: MESOS-5779
>     https://issues.apache.org/jira/browse/MESOS-5779
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The `docker::spec::v1::ImageManifest` protobuf implements the
> official v1 image manifest specification found at:
> 
> https://github.com/docker/docker/blob/master/image/spec/v1.md
> 
> The field names in this spec are all written in snake_case as are the
> field names of the JSON representing the image manifest when reading
> it from disk (for example after performing a `docker save`). As such,
> the protobuf for ImageManifest also provides these fields in
> snake_case. Unfortunately, the `docker inspect` command also provides
> a method of retrieving the JSON for an image manifest, with one major
> caveat -- it represents all of its top level keys in CamelCase.
> 
> To allow both representations to be parsed in the same way, we
> intercept the incoming JSON from either source (disk or `docker
> inspect`) and convert it to a canonical snake_case representation.
> 
> 
> Diffs
> -----
> 
>   src/docker/spec.cpp 2711578626dd1847f73cbf7bd3771f36e6755a99 
>   src/tests/containerizer/docker_tests.cpp 49bd7c252c342c8cb29ea916ad3f1f71638d2017 
> 
> Diff: https://reviews.apache.org/r/49598/diff/
> 
> 
> Testing
> -------
> 
> `sudo GTEST_FILTER="*ROOT_DOCKER_CompareImageManifests" bin/mesos-tests.sh`
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>


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