Address reviewer's comments.

- fix unit tests'name
- have _evalute_merge_env to properly expand envvar
- magic_path -> _magic_path to make it private
- add comments for provider usage
- create alias docker_layer / oci_layer

container/image.bzl

@@ -165,6 +165,23 @@ def _repository_name(ctx):
 def _getLayerInfo(layers, info):
   return [getattr(layer[LayerInfo], info) for layer in layers]
 
+def _evalute_merge_env(env, new_env):
+  """Expand new environment variables properly and merge them.
+
+  Example:
+  - input: env={'foo':'bar', 'PATH':'$PATH:a:b'}
+           new_env={'X':'Y', 'PATH':'$PATH:c' }
+  - output: env={'foo':'bar', 'X':'Y', 'PATH':'$PATH:a:b:c'}
+  """
+  for k, v in new_env.items():
+    elems = []
+    for e in v.split(":"):
+      if e.startswith("$") and e[1:] in env.keys():
+        elems.append(env.get(e[1:]))
+      else:
+        elems.append(e)
+    env[k] = ":".join(elems)
+
 def _impl(ctx, files=None, file_map=None, empty_files=None, directory=None,
           entrypoint=None, cmd=None, symlinks=None, output=None, env=None,
           container_layers=None, debs=None, tars=None):
@@ -219,8 +236,8 @@ def _impl(ctx, files=None, file_map=None, empty_files=None, directory=None,
 
   # Get and merge environment variables
   env = {}
-  [env.update(layer[LayerInfo].env) for layer in layers]
-  env.update(ctx.attr.env)
+  [_evalute_merge_env(env, layer[LayerInfo].env) for layer in layers]
+  _evalute_merge_env(env, ctx.attr.env)
 
   # Generate the new config using the attributes specified and the diff_id
   config_file, config_digest = _image_config(

container/image_test.py

@@ -66,7 +66,7 @@ def test_files_with_file_base(self):
       self.assertEqual(2, len(img.fs_layers()))
       self.assertTopLayerContains(img, ['.', './bar'])
 
-  def test_files_with_file_base(self):
+  def test_files_in_layer_with_file_base(self):
     with TestImage('files_in_layer_with_files_base') as img:
       self.assertDigest(img, '4b008d8241bdbbe930d72d8f0ee7b61d11561946db0fd52d02dbcb8842b3a958')
       self.assertEqual(3, len(img.fs_layers()))
@@ -91,7 +91,7 @@ def test_tar_with_tar_base(self):
         './asdf', './usr', './usr/bin',
         './usr/bin/miraclegrow'])
 
-  def test_tar_with_tar_base(self):
+  def test_tars_in_layer_with_tar_base(self):
     with TestImage('tars_in_layer_with_tar_base') as img:
       self.assertDigest(img, '80f850359828f763ae544f9b7725f89755f1a28a80738a514735becae60924af')
       self.assertEqual(3, len(img.fs_layers()))
@@ -189,9 +189,9 @@ def test_with_env(self):
 
   def test_layers_with_env(self):
     with TestImage('layers_with_env') as img:
-      self.assertDigest(img, 'c98768980847b394c4e194c7041662703e20a07925c8db78c12f2bcb2b09c926')
+      self.assertDigest(img, 'ecab0f39b4726e69c62747ce4c1662f697060eef23a26db719a47ea379b77d7f')
       self.assertEqual(3, len(img.fs_layers()))
-      self.assertConfigEqual(img, 'Env', ['abc=ABC', 'xyz=xyz'])
+      self.assertConfigEqual(img, 'Env', ['PATH=$PATH:/tmp/a:/tmp/b:/tmp/c', 'a=b', 'x=y'])
 
   def test_dummy_repository(self):
     # We allow users to specify an alternate repository name instead of 'bazel/'

container/layer.bzl

@@ -43,7 +43,7 @@ load(
     _join_path = "join",
 )
 
-def magic_path(ctx, f):
+def _magic_path(ctx, f):
   # Right now the logic this uses is a bit crazy/buggy, so to support
   # bug-for-bug compatibility in the foo_image rules, expose the logic.
   # See also: https://github.com/bazelbuild/rules_docker/issues/106
@@ -70,7 +70,7 @@ def build_layer(ctx, files=None, file_map=None, empty_files=None,
       "--mode=" + ctx.attr.mode,
   ]
 
-  args += ["--file=%s=%s" % (f.path, magic_path(ctx, f)) for f in files]
+  args += ["--file=%s=%s" % (f.path, _magic_path(ctx, f)) for f in files]
   args += ["--file=%s=%s" % (f.path, path) for (path, f) in file_map.items()]
   args += ["--empty_file=%s" % f for f in empty_files or []]
   args += ["--tar=" + f.path for f in tars]
@@ -127,7 +127,12 @@ def _impl(ctx, files=None, file_map=None, empty_files=None, directory=None,
                                         debs=debs, tars=tars)
   # Generate the zipped filesystem layer, and its sha256 (aka blob sum)
   zipped_layer, blob_sum = zip_layer(ctx, unzipped_layer)
-  # Returns constituent parts of the Container layer as provider
+  # Returns constituent parts of the Container layer as provider:
+  # - in container_image rule, we need to use all the following information,
+  #   e.g. zipped_layer etc., to assemble the complete container image.
+  # - in order to expose information from container_layer rule to container_image
+  #   rule, they need to be packaged into a provider, see:
+  #   https://docs.bazel.build/versions/master/skylark/rules.html#providers
   return [LayerInfo(zipped_layer=zipped_layer,
                     blob_sum=blob_sum,
                     unzipped_layer=unzipped_layer,

docker/docker.bzl

@@ -20,6 +20,7 @@ load(
     docker_bundle = "container_bundle",
     docker_flatten = "container_flatten",
     docker_image = "container_image",
+    docker_layer = "container_layer",
     docker_import = "container_import",
     docker_load = "container_load",
     docker_pull = "container_pull",

oci/oci.bzl

@@ -19,6 +19,7 @@ load(
     oci_bundle = "container_bundle",
     oci_flatten = "container_flatten",
     oci_image = "container_image",
+    oci_layer = "container_layer",
     oci_import = "container_import",
     oci_load = "container_load",
     oci_pull = "container_pull",

testdata/BUILD

@@ -266,15 +266,16 @@ container_image(
     base = ":base_with_volume",
     container_layers = [":env_layer"],
     env = {
-        "abc": "ABC",
+        "PATH": "$PATH:/tmp/b:/tmp/c",
+        "x": "y",
     },
 )
 
 container_layer(
     name = "env_layer",
     env = {
-        "abc": "abc",
-        "xyz": "xyz",
+        "PATH": "$PATH:/tmp/a",
+        "a": "b",
     },
 )