fixes naming when project has multiple requirements.txt files

Author: edublancas

.gitignore

@@ -1,3 +1,6 @@
+# folder with examples using "kind" - we can delete it once we moved them to docs
+kind
+
 *.sublime-project
 *.sublime-workspace
 *.source

setup.cfg

@@ -1,2 +1,3 @@
 [flake8]
-exclude = build/, src/soopervisor/assets/aws-lambda/app.py, tests/assets/fast-pipeline/fast_pipeline.py
+# kind is a tmp directory with tutorials, we can delete it once we move their content to the docs
+exclude = build/, src/soopervisor/assets/aws-lambda/app.py, tests/assets/fast-pipeline/fast_pipeline.py, kind/

src/soopervisor/aws/batch.py

@@ -144,14 +144,15 @@ def _submit_dag(
     # Register job definitions for task specific images
     for pattern, image in image_map.items():
         if pattern != default_image_key:
-            container_properties['image'] = image
+            container_props = container_properties.copy()
+            container_props['image'] = image
             task_job_def = f"{job_def}-{docker.modify_wildcard(pattern)}"
             cmdr.info(f'Registering {task_job_def!r} job definition...')
 
             jd = client.register_job_definition(
                 jobDefinitionName=task_job_def,
                 type='container',
-                containerProperties=container_properties)
+                containerProperties=container_props)
             jd_map[pattern] = jd
 
     for name, upstream in tasks.items():
@@ -280,6 +281,7 @@ def _export(cls, cfg, env_name, mode, until, skip_tests, skip_docker,
 
             # add a unique suffix to prevent collisions
             suffix = str(uuid4())[:8]
+
             cls._submit_dag(tasks=tasks,
                             args=cli_args,
                             job_def=f'{pkg_name}-{suffix}',

src/soopervisor/commons/docker.py

@@ -1,3 +1,5 @@
+import os
+import shlex
 import tarfile
 from pathlib import Path
 
@@ -50,21 +52,42 @@ def get_dependencies():
     return dependency_files, lock_paths
 
 
-def build_image(e, cfg, env_name, until, entry_point, skip_tests, ignore_git,
-                pkg_name, version, task):
+def build_image(
+    e,
+    cfg,
+    env_name,
+    until,
+    entry_point,
+    skip_tests,
+    ignore_git,
+    pkg_name,
+    version,
+    task,
+):
+    """Build Docker image
+
+    Parameters
+    ----------
+    pkg_name : str
+        Project name, used to tag the Docker image (e.g. {pkg_name}:{version})
+
+    version : str
+        Project version, user to tag the Docker image
+        (e.g. {pkg_name}:{version})
+    """
 
     e.cp('dist')
 
     e.cd(env_name)
 
     if task:
-        image_local = f'{pkg_name}:{version}-{modify_wildcard(task)}'
-
-    import os
-    import shlex
+        suffix = '' if task == 'default' else f'-{modify_wildcard(task)}'
+        image_local = f'{pkg_name}{suffix}:{version}'
 
     args = ['docker', 'build', '.', '--tag', image_local]
 
+    # NOTE: this is used in Ploomber Cloud, but it isn't documented in
+    # soopervisor's docs
     if 'DOCKER_ARGS' in os.environ:
         args = args + shlex.split(os.environ['DOCKER_ARGS'])
 
@@ -111,9 +134,11 @@ def build_image(e, cfg, env_name, until, entry_point, skip_tests, ignore_git,
 
     if cfg.repository:
         image_target = cfg.repository
+
         # Adding the latest tag if not a remote repo
         if ":" not in image_target:
-            image_target = f'{image_target}:{version}-{modify_wildcard(task)}'
+            image_target = f'{image_target}{suffix}:{version}'
+
         e.run('docker',
               'tag',
               image_local,
@@ -176,7 +201,8 @@ def build(e,
 
     # Generate source distribution
     if setup_flow:
-        for task_pattern in list(dependency_files.keys()):
+        for task_pattern in sorted(dependency_files.keys()):
+
             if task_pattern != 'default':
                 raise NotImplementedError(
                     "Multiple requirements.*.lock.txt or "
@@ -188,9 +214,11 @@ def build(e,
             e.cp('requirements.lock.txt')
         elif Path('environment.lock.yml').exists():
             e.cp('environment.lock.yml')
+
         e.rm('dist', 'build', Path('src', pkg_name, f'{pkg_name}.egg-info'))
         e.run('python', '-m', 'build', '--sdist', description='Packaging code')
         default_image_key = dependencies.get_default_image_key()
+
         image = build_image(e,
                             cfg,
                             env_name,
@@ -206,7 +234,10 @@ def build(e,
         # raise error if include is not None? and suggest to use MANIFEST.in
         # instead
     else:
-        for task, lock_file in lock_paths.items():
+        # sort keys so we iterate in deterministic order and can test easily
+        for task in sorted(lock_paths.keys()):
+            lock_file = lock_paths[task]
+
             if Path(lock_file).exists():
                 e.cp(lock_file)
             e.rm('dist')

tests/airflow/test_airflow_export.py

@@ -174,6 +174,5 @@ def test_export_airflow_callables(monkeypatch, mock_docker_calls_callables,
     assert td['fit'].arguments == [template.format('fit') + args]
     assert td['join'].arguments == [template.format('join') + args]
 
-    assert {t.image
-            for t in td.values()} == {'your-repository/name:latest-default'}
+    assert {t.image for t in td.values()} == {'your-repository/name:latest'}
     assert {tuple(t.cmds) for t in td.values()} == {('bash', '-cx')}

tests/argo/test_argo_export.py

@@ -62,7 +62,7 @@ def test_export(mock_docker_my_project, backup_packaged_project, monkeypatch,
     assert run_task_template['script']['workingDir'] is None
 
     assert run_task_template['script'][
-        'image'] == 'your-repository/name:0.1dev-default'
+        'image'] == 'your-repository/name:0.1dev'
     assert run_task_template['name'] == 'run-task'
     assert spec['metadata']['generateName'] == 'my-project-'
     assert all([

tests/aws_batch/test_export.py

@@ -329,13 +329,13 @@ def test_export_multiple_images_job_details(
     assert job_images == {
         'arn:aws:batch:us-east-1:123456789012:job-definition/'
         'multiple_requirements_project-uuid4:1':
-        'your-repository/name:latest-default',
+        'your-repository/name:latest',
         'arn:aws:batch:us-east-1:123456789012:job-definition/'
         'multiple_requirements_project-uuid4-clean-ploomber:1':
-        'your-repository/name:latest-clean-ploomber',
+        'your-repository/name-clean-ploomber:latest',
         'arn:aws:batch:us-east-1:123456789012:job-definition/'
         'multiple_requirements_project-uuid4-plot-ploomber:1':
-        'your-repository/name:latest-plot-ploomber',
+        'your-repository/name-plot-ploomber:latest',
     }
 
 
@@ -410,8 +410,8 @@ def test_checks_the_right_spec(mock_batch, mock_docker_my_project_serve,
     exporter.add()
     exporter.export(mode='incremental')
 
-    expected = ('docker', 'run', 'my_project:0.1dev-default', 'ploomber',
-                'status', '--entry-point',
+    expected = ('docker', 'run', 'my_project:0.1dev', 'ploomber', 'status',
+                '--entry-point',
                 str(Path('src', 'my_project', 'pipeline.serve.yaml')))
     assert mock_docker_my_project_serve.calls[2] == expected
 

tests/conftest.py

@@ -27,9 +27,14 @@ class CustomCommander(_commander.Commander):
     CustomCommander.run('docker', ...)
     """
 
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.docker_cmds = []
+
     def run(self, *args, **kwargs):
         if args[0] == 'docker':
             print(f'ignoring: {args} {kwargs}')
+            self.docker_cmds.append([args, kwargs])
         else:
             return super().run(*args, **kwargs)
 
@@ -47,6 +52,7 @@ def git_init():
 
 # mocks subprocess.run()
 def subprocess_run_mock(tester):
+
     def ret(cmd, capture_output):
         result = tester(cmd)
 
@@ -63,7 +69,7 @@ def ret(cmd, capture_output):
     return ret
 
 
-def _mock_commander_calls(monkeypatch, cmd, proj, tag, task='default'):
+def _mock_commander_calls(monkeypatch, cmd, proj, tag, task=None):
     """
     Mock subprocess calls made by ploomber.io._commander.Commander (which
     is used interally by the soopervisor.commons.docker module).
@@ -73,12 +79,14 @@ def _mock_commander_calls(monkeypatch, cmd, proj, tag, task='default'):
     to "python -m build --sdist" to pass. For details, see the CommanderTester
     docstring in the ploomber package.
     """
+    suffix = '' if task is None else f'-{task}'
+
     tester = _commander_tester.CommanderTester(
         run=[
             ('python', '-m', 'build', '--sdist'),
         ],
         return_value={
-            ('docker', 'run', f'{proj}:{tag}-{task}', 'python', '-c', cmd):
+            ('docker', 'run', f'{proj}{suffix}:{tag}', 'python', '-c', cmd):
             b'True\n'
         })
 

tests/test_commons.py

@@ -626,6 +626,47 @@ def test_docker_build(tmp_sample_project):
 
     assert existing == expected
 
+    # check tag name
+    cmd = cmdr.docker_cmds[0][0]
+    name, tag = cmd[1], cmd[-1]
+    assert name == 'build' and tag == 'sample_project:latest'
+
+
+@pytest.mark.parametrize('repo, expected', [
+    ['docker.company.com/something', 'docker.company.com/something:latest'],
+    ['docker.company.com/something:v2', 'docker.company.com/something:v2'],
+])
+def test_docker_build_with_repository(tmp_sample_project, repo, expected):
+    Path('some-env').mkdir()
+    Path('some-env', 'Dockerfile').touch()
+
+    cfg = ConcreteDockerConfig(repository=repo)
+
+    with CustomCommander(workspace='some-env') as cmdr:
+        commons.docker.build(cmdr,
+                             cfg,
+                             'some-env',
+                             until=None,
+                             entry_point='pipeline.yaml')
+    # check tag name
+    cmd = cmdr.docker_cmds[0][0]
+    assert cmd == (
+        'docker',
+        'build',
+        '.',
+        '--tag',
+        'sample_project:latest',
+    )
+
+    # check tag command
+    cmd = cmdr.docker_cmds[-2][0]
+    assert cmd == (
+        'docker',
+        'tag',
+        'sample_project:latest',
+        expected,
+    )
+
 
 def test_docker_build_multiple_requirement(
         tmp_sample_project_multiple_requirement):
@@ -639,11 +680,53 @@ def test_docker_build_multiple_requirement(
                                  'some-env',
                                  until=None,
                                  entry_point='pipeline.yaml')
+
+    # validate docker is called with the right arguments
+    docker_args = [args[0] for args in cmdr.docker_cmds]
+
+    def generate_commands(suffix):
+        image_name = f'multiple_requirements_project{suffix}:latest'
+
+        return [
+            # build image
+            (
+                'docker',
+                'build',
+                '.',
+                '--tag',
+                image_name,
+            ),
+            # check pipeline load
+            (
+                'docker',
+                'run',
+                image_name,
+                'ploomber',
+                'status',
+                '--entry-point',
+                'pipeline.yaml',
+            ),
+            # check pipeline has a client
+            (
+                'docker',
+                'run',
+                image_name,
+                'python',
+                '-c',
+                ('from ploomber.spec import DAGSpec; '
+                 'print("File" in DAGSpec("pipeline.yaml").to_dag().clients)'),
+            )
+        ]
+
+    assert generate_commands('-clean-ploomber') == docker_args[:3]
+    assert generate_commands('') == docker_args[3:6]
+    assert generate_commands('-plot-ploomber') == docker_args[6:]
+
     assert pkg_name == 'multiple_requirements_project'
     assert image_map == \
-           {'default': 'multiple_requirements_project:latest-default',
-            'clean-*': 'multiple_requirements_project:latest-clean-ploomber',
-            'plot-*': 'multiple_requirements_project:latest-plot-ploomber'}
+           {'default': 'multiple_requirements_project:latest',
+            'clean-*': 'multiple_requirements_project-clean-ploomber:latest',
+            'plot-*': 'multiple_requirements_project-plot-ploomber:latest'}
 
     existing = _list_files(Path('dist',
                                 'multiple_requirements_project.tar.gz'))