Merge pull request #320 from demisto/sdk-docker-fix

Docker image tag validation login change

CHANGELOG.md

@@ -6,6 +6,8 @@
 * Missing release notes message are now showing the release notes file path to update.
 * Fixed an issue in the **validate** command in which unified YAML files were not ignored.
 * File format suggestions are now shown in the relevant file format (JSON or YAML).
+* Changed Docker image validation to fail only on non-valid ones.
+* Removed backward compatibility validation when Docker image is updated.
 
 #### 1.0.0
 * Improved the *upload* command to support the upload of all the content entities within a pack.

demisto_sdk/commands/common/constants.py

@@ -128,17 +128,6 @@ def breaking_backwards_command(cls, file_path, old_command):
         return "{}: {}, You've changed the context in the file,please " \
                "undo. the command is:\n{}".format(file_path, cls.BACKWARDS, old_command)
 
-    @classmethod
-    def breaking_backwards_docker(cls, file_path, old_docker, new_docker):
-        return "{}: {}, You've changed the docker for the file," \
-               " this is not allowed. Old: {}, New: {} ".format(file_path, cls.BACKWARDS, old_docker, new_docker)
-
-    @staticmethod
-    def not_latest_docker(file_path, current_docker, latest_docker):
-        return "{}: You're not using latest docker for the file," \
-               " please update to latest version. Current: {}, Latest: {} ".format(file_path, current_docker,
-                                                                                   latest_docker)
-
     @classmethod
     def breaking_backwards_arg_changed(cls, file_path):
         return "{}: {}, You've changed the name of an arg in " \

demisto_sdk/commands/common/hook_validations/docker.py

@@ -1,11 +1,9 @@
 import re
 from datetime import datetime, timedelta
-from distutils.version import LooseVersion
 
 from pkg_resources import parse_version
 
 import requests
-from demisto_sdk.commands.common.constants import Errors
 from demisto_sdk.commands.common.tools import (get_yaml, print_error,
                                                print_warning)
 
@@ -42,36 +40,44 @@ def is_docker_image_valid(self):
         if not self.docker_image_latest_tag:
             self.is_valid = False
         elif not self.is_docker_image_latest_tag():
-            print_error(Errors.not_latest_docker(self.file_path, self.docker_image_tag, self.docker_image_latest_tag))
             self.is_valid = False
         return self.is_valid
 
     def is_docker_image_latest_tag(self):
+        if 'demisto/python:1.3-alpine' == f'{self.docker_image_name}:{self.docker_image_tag}':
+            # the docker image is the default one
+            self.is_latest_tag = False
+            print_error('The current docker image in the yml file is the default one: demisto/python:1.3-alpine,\n'
+                        'Please create or use another docker image\n')
+            return self.is_latest_tag
+
         if not self.docker_image_name or not self.docker_image_latest_tag:
             # If the docker image isn't in the format we expect it to be or we failed fetching the tag
             # We don't want to print any error msgs to user because they have already been printed
             self.is_latest_tag = False
             return self.is_latest_tag
 
-        server_version = LooseVersion(self.from_version)
-        # Case of a modified file with version >= 5.0.0
-        if self.is_modified_file and server_version >= '5.0.0':
-            if self.docker_image_latest_tag != self.docker_image_tag and not \
-                    'demisto/python:1.3-alpine' == '{}:{}'.format(self.docker_image_name, self.docker_image_tag):
-                # If docker image name are different and if the docker image isn't the default one
-                self.is_latest_tag = False
-        # Case of an added file
-        elif not self.is_modified_file:
-            if self.docker_image_latest_tag != self.docker_image_tag:
-                self.is_latest_tag = False
+        if self.docker_image_latest_tag != self.docker_image_tag:
+            # If docker image tag is not the most updated one that exists in docker-hub
+            self.is_latest_tag = False
 
         if not self.is_latest_tag:
-            print_error('The docker image tag is not the latest, please update it.\n'
-                        'The docker image tag in the yml file is: {}\n'
-                        'The latest docker image tag in docker hub is: {}\n'
-                        'You can check for the tags of {} here: https://hub.docker.com/r/{}/tags\n'
-                        .format(self.docker_image_tag, self.docker_image_latest_tag, self.docker_image_name,
-                                self.docker_image_name))
+            self.is_latest_tag = False
+            print_warning('The docker image tag is not the latest, please update it.\n'
+                          'The docker image tag in the yml file is: {}\n'
+                          'The latest docker image tag in docker hub is: {}\n'
+                          'You can check for the most updated version of {} here: https://hub.docker.com/r/{}/tags\n'
+                          .format(self.docker_image_tag, self.docker_image_latest_tag, self.docker_image_name,
+                                  self.docker_image_name))
+
+        # the most updated tag should be numeric and not labeled "latest"
+        if self.docker_image_latest_tag == "latest":
+            self.is_latest_tag = False
+            print_error('"latest" tag is not allowed,\n'
+                        'Please create or update to an updated versioned image\n'
+                        'You can check for the most updated version of {} here: https://hub.docker.com/r/{}/tags\n'
+                        .format(self.docker_image_tag, self.docker_image_name))
+
         return self.is_latest_tag
 
     def get_docker_image_from_yml(self):
@@ -169,9 +175,8 @@ def lexical_find_latest_tag(tags):
         return max_tag
 
     @staticmethod
-    def find_latest_tag_by_date(tags):
+    def find_latest_tag_by_date(tags: list) -> str:
         """Get the latest tags by datetime comparison.
-
         Args:
             tags(list): List of dictionaries representing the docker image tags
 
@@ -182,10 +187,9 @@ def find_latest_tag_by_date(tags):
         latest_tag_date = datetime.now() - timedelta(days=400000)
         for tag in tags:
             tag_date = datetime.strptime(tag.get('last_updated'), '%Y-%m-%dT%H:%M:%S.%fZ')
-            if tag_date >= latest_tag_date:
+            if tag_date >= latest_tag_date and tag.get('name') != 'latest':
                 latest_tag_date = tag_date
                 latest_tag_name = tag.get('name')
-
         return latest_tag_name
 
     @staticmethod
@@ -277,5 +281,5 @@ def parse_docker_image(docker_image):
 
             return image, tag
         else:
-            # If the yml file has no docker image we provide the default one 'demisto/python:1.3-alpine'
-            return 'demisto/python', '1.3-alpine'
+            # If the yml file has no docker image we provide a default one with numeric tag
+            return 'demisto/python', '2.7.17.6981'

demisto_sdk/commands/common/hook_validations/integration.py

@@ -15,8 +15,7 @@
     DockerImageValidator
 from demisto_sdk.commands.common.hook_validations.image import ImageValidator
 from demisto_sdk.commands.common.hook_validations.utils import is_v2_file
-from demisto_sdk.commands.common.tools import (get_dockerimage45, print_error,
-                                               print_warning,
+from demisto_sdk.commands.common.tools import (print_error, print_warning,
                                                server_version_compare)
 
 
@@ -44,7 +43,6 @@ def is_backward_compatible(self):
 
         answers = [
             self.is_changed_context_path(),
-            self.is_docker_image_changed(),
             self.is_added_required_fields(),
             self.is_changed_command_name_or_arg(),
             self.is_there_duplicate_args(),
@@ -487,19 +485,6 @@ def is_added_required_fields(self):
                 is_added_required = True
         return is_added_required
 
-    def is_docker_image_changed(self):
-        """Check if the Docker image was changed or not."""
-        # Unnecessary to check docker image only on 5.0 and up
-        if server_version_compare(self.old_file.get('fromversion', '0'), '5.0.0') < 0:
-            old_docker = get_dockerimage45(self.old_file.get('script', {}))
-            new_docker = get_dockerimage45(self.current_file.get('script', {}))
-            if old_docker != new_docker:
-                print_error(Errors.breaking_backwards_docker(self.file_path, old_docker, new_docker))
-                self.is_valid = False
-                return True
-
-        return False
-
     def is_id_equals_name(self):
         """Check whether the integration's ID is equal to its name
 

demisto_sdk/commands/common/hook_validations/script.py

@@ -5,7 +5,7 @@
 from demisto_sdk.commands.common.hook_validations.docker import \
     DockerImageValidator
 from demisto_sdk.commands.common.hook_validations.utils import is_v2_file
-from demisto_sdk.commands.common.tools import (get_dockerimage45, print_error,
+from demisto_sdk.commands.common.tools import (print_error,
                                                server_version_compare)
 
 
@@ -37,7 +37,6 @@ def is_backward_compatible(self):
             return True
 
         is_breaking_backwards = [
-            self.is_docker_image_changed(),
             self.is_context_path_changed(),
             self.is_added_required_args(),
             self.is_arg_changed(),
@@ -153,18 +152,6 @@ def is_context_path_changed(self):
             return True
         return False
 
-    def is_docker_image_changed(self):
-        # type: () -> bool
-        """Check if the docker image as been changed."""
-        # Unnecessary to check docker image only on 5.0 and up
-        if server_version_compare(self.old_file.get('fromversion', '0'), '5.0.0') < 0:
-            old_docker = get_dockerimage45(self.old_file)
-            new_docker = get_dockerimage45(self.current_file)
-            if old_docker != new_docker:
-                print_error(Errors.breaking_backwards_docker(self.file_path, old_docker, new_docker))
-                return True
-        return False
-
     def is_id_equals_name(self):
         """Check whether the script's ID is equal to its name
 

demisto_sdk/commands/common/tests/docker_test.py

@@ -1,11 +1,12 @@
 import os
+from unittest.mock import patch
 
+import mock
 import pytest
 from demisto_sdk.commands.common.git_tools import git_path
 from demisto_sdk.commands.common.hook_validations.docker import \
     DockerImageValidator
 from demisto_sdk.commands.common.tools import get_yaml
-from mock import patch
 
 RETURN_ERROR_TARGET = 'GetDockerImageLatestTag.return_error'
 
@@ -132,52 +133,122 @@ def test_parse_docker_image():
 
 
 # disable-secrets-detection-end
-
-
-def test_is_docker_image_latest_tag():
-    with patch.object(DockerImageValidator, '__init__', lambda x, y, z, w: None):
+def test_is_docker_image_latest_tag_with_default_image():
+    """
+    Given
+    - The default docker image - 'demisto/python:1.3-alpine'
+
+    When
+    - The most updated docker image in docker-hub is '1.0.3'
+
+    Then
+    -  If the docker image is numeric and the most update one, it is Valid
+    -  If the docker image is not numeric and labeled "latest", it is Invalid
+   """
+    with mock.patch.object(DockerImageValidator, '__init__', lambda x, y, z, w: None):
         docker_image_validator = DockerImageValidator(None, None, None)
         docker_image_validator.yml_file = {}
-        docker_image_validator.docker_image_latest_tag = 'latest_tag'
+        docker_image_validator.docker_image_latest_tag = '1.0.3'
         docker_image_validator.docker_image_name = 'demisto/python'
-        docker_image_validator.from_version = '5.0.0'
 
-        # ===== Added File Tests =====
-        # default docker image
         docker_image_validator.is_latest_tag = True
         docker_image_validator.is_modified_file = False
         docker_image_validator.docker_image_tag = '1.3-alpine'
         assert docker_image_validator.is_docker_image_latest_tag() is False
+        assert docker_image_validator.is_latest_tag is False
+
+
+def test_is_docker_image_latest_tag_with_tag_labeled_latest():
+    """
+    Given
+    - A docker image with "latest" as tag
+
+    When
+    - The most updated docker image in docker-hub is '1.0.3'
+
+    Then
+    -  If the docker image is numeric and the most update one, it is Valid
+    -  If the docker image is not numeric and labeled "latest", it is Invalid
+   """
+    with mock.patch.object(DockerImageValidator, '__init__', lambda x, y, z, w: None):
+        docker_image_validator = DockerImageValidator(None, None, None)
+        docker_image_validator.yml_file = {}
+        docker_image_validator.docker_image_latest_tag = 'latest'
+        docker_image_validator.docker_image_name = 'demisto/python'
 
-        # regular docker image, not latest tag
         docker_image_validator.is_latest_tag = True
-        docker_image_validator.docker_image_tag = 'not_latest_tag'
+        docker_image_validator.docker_image_tag = 'latest'
         assert docker_image_validator.is_docker_image_latest_tag() is False
+        assert docker_image_validator.is_latest_tag is False
 
-        # regular docker image, latest tag
-        docker_image_validator.is_latest_tag = True
-        docker_image_validator.docker_image_tag = 'latest_tag'
-        assert docker_image_validator.is_docker_image_latest_tag() is True
 
-        # ===== Modified File Tests =====
-        # from version 4.1.0
-        docker_image_validator.is_latest_tag = True
-        docker_image_validator.is_modified_file = True
-        docker_image_validator.from_version = '4.1.0'
-        assert docker_image_validator.is_docker_image_latest_tag() is True
+def test_is_docker_image_latest_tag_with_latest_tag():
+    """
+   Given
+   - A docker image with '1.0.3' as tag
+
+   When
+   - The most updated docker image in docker-hub is '1.0.3'
+
+   Then
+   -  If the docker image is numeric and the most update one, it is Valid
+   -  If the docker image is not numeric and labeled "latest", it is Invalid
+  """
+    with mock.patch.object(DockerImageValidator, '__init__', lambda x, y, z, w: None):
+        docker_image_validator = DockerImageValidator(None, None, None)
+        docker_image_validator.yml_file = {}
+        docker_image_validator.docker_image_latest_tag = '1.0.3'
+        docker_image_validator.docker_image_name = 'demisto/python'
 
-        # from version 5.0.0 - regular docker image, latest tag
         docker_image_validator.is_latest_tag = True
-        docker_image_validator.from_version = '5.0.0'
+        docker_image_validator.docker_image_tag = '1.0.3'
         assert docker_image_validator.is_docker_image_latest_tag() is True
+        assert docker_image_validator.is_latest_tag is True
+
+
+def test_is_docker_image_latest_tag_with_numeric_but_not_most_updated():
+    """
+   Given
+   - A docker image with '1.0.2' as tag
+
+   When
+   - The most updated docker image in docker-hub is '1.0.3'
+
+   Then
+   -  If the docker image is numeric and the most update one, it is Valid
+   -  If the docker image is not numeric and labeled "latest", it is Invalid
+  """
+    with mock.patch.object(DockerImageValidator, '__init__', lambda x, y, z, w: None):
+        docker_image_validator = DockerImageValidator(None, None, None)
+        docker_image_validator.yml_file = {}
+        docker_image_validator.docker_image_latest_tag = '1.0.3'
+        docker_image_validator.docker_image_name = 'demisto/python'
 
-        # from version 5.0.0 - regular docker image, not latest tag
         docker_image_validator.is_latest_tag = True
-        docker_image_validator.from_version = '5.0.0'
-        docker_image_validator.docker_image_tag = 'not_latest_tag'
+        docker_image_validator.docker_image_tag = '1.0.2'
         assert docker_image_validator.is_docker_image_latest_tag() is False
+        assert docker_image_validator.is_latest_tag is False
+
+
+def test_is_docker_image_latest_tag_without_tag():
+    """
+   Given
+   - A latest docker image has an empty tag
+
+   When
+   - The most updated docker image in docker-hub is '1.0.3'
+
+   Then
+   -  If the docker image is numeric and the most update one, it is Valid
+   -  If the docker image is not numeric and labeled "latest", it is Invalid
+  """
+    with mock.patch.object(DockerImageValidator, '__init__', lambda x, y, z, w: None):
+        docker_image_validator = DockerImageValidator(None, None, None)
+        docker_image_validator.yml_file = {}
+        docker_image_validator.docker_image_latest_tag = ''
+        docker_image_validator.docker_image_name = 'demisto/python'
 
-        # from version 5.0.0 - default docker image
         docker_image_validator.is_latest_tag = True
-        docker_image_validator.docker_image_tag = '1.3-alpine'
-        assert docker_image_validator.is_docker_image_latest_tag() is True
+        docker_image_validator.docker_image_tag = '1.0.2'
+        assert docker_image_validator.is_docker_image_latest_tag() is False
+        assert docker_image_validator.is_latest_tag is False

demisto_sdk/commands/common/tests/integration_test.py

@@ -38,12 +38,6 @@ class TestIntegrationValidator:
         (SCRIPT_WITH_DOCKER_IMAGE_1, EMPTY_CASE, True)
     ]
 
-    @pytest.mark.parametrize("current_file, old_file, answer", IS_DOCKER_IMAGE_CHANGED)
-    def test_is_docker_image_changed(self, current_file, old_file, answer):
-        structure = mock_structure("", current_file, old_file)
-        validator = IntegrationValidator(structure)
-        assert validator.is_docker_image_changed() is answer
-
     REQUIED_FIELDS_FALSE = {"configuration": [{"name": "test", "required": False}]}
     REQUIED_FIELDS_TRUE = {"configuration": [{"name": "test", "required": True}]}
     IS_ADDED_REQUIRED_FIELDS_INPUTS = [

demisto_sdk/commands/common/tests/script_test.py

@@ -42,11 +42,6 @@ class TestScriptValidator:
         (BASE_DOCKER_IMAGE, UPDATED_DOCKER_IMAGE, True),
     ]
 
-    @pytest.mark.parametrize('current_file, old_file, answer', INPUTS_DOCKER_IMAGES)
-    def test_is_docker_image_changed(self, current_file, old_file, answer):
-        validator = get_validator(current_file, old_file)
-        assert validator.is_docker_image_changed() is answer
-
     SANE_DOC_PATH = 'Scripts/SaneDocReport/SaneDocReport.yml'
     SANE_DOC_SUBTYPE = {
         "type": "python",