From 5d3ba2d58b731c629c99c5e435da047fb219c5ac Mon Sep 17 00:00:00 2001 From: dmeylakh Date: Thu, 13 Jun 2024 19:27:57 +0300 Subject: [PATCH 1/5] committing addition --- .../commands/validate/default_config.toml | 4 +- demisto_sdk/commands/validate/initializer.py | 28 +++++++++ .../validate/sdk_validation_config.toml | 2 +- .../validate/tests/PA_validators_test.py | 32 ++++++++++ .../validate/tests/validators_test.py | 50 ++++++++++++++- .../commands/validate/validate_manager.py | 9 +++ ..._pack_metadata_version_should_be_raised.py | 61 +++++++++++++++++++ .../validate/validators/base_validator.py | 1 + 8 files changed, 183 insertions(+), 4 deletions(-) create mode 100644 demisto_sdk/commands/validate/validators/PA_validators/PA114_pack_metadata_version_should_be_raised.py diff --git a/demisto_sdk/commands/validate/default_config.toml b/demisto_sdk/commands/validate/default_config.toml index 6b4ef9fc9a..8c2ab18a6f 100644 --- a/demisto_sdk/commands/validate/default_config.toml +++ b/demisto_sdk/commands/validate/default_config.toml @@ -12,7 +12,7 @@ select = [ "BA101", "BA105", "BA106", "BA116", "BA118", "DO104", "IN108", "IN130", - "PA107", "PA108", "PA111", "PA115", "PA128", "PA130", + "PA107", "PA108", "PA111", "PA114", "PA115", "PA128", "PA130", "RP101", ] warning = [] @@ -32,7 +32,7 @@ warning = [] [xsoar_best_practices_use_git] select = [ "BA100", "BA101", "BA105", "BA106", "BA110", "BA111", "BA116", "BA118", "BA124", "BA126", - "PA100", "PA101", "PA102", "PA103", "PA104", "PA105", "PA107", "PA108", "PA109", "PA111", "PA113", "PA115", "PA117", "PA118", "PA119", "PA120", + "PA100", "PA101", "PA102", "PA103", "PA104", "PA105", "PA107", "PA108", "PA109", "PA111", "PA113", "PA114", "PA115", "PA117", "PA118", "PA119", "PA120", "PA121", "PA123", "PA125", "PA127", "PA128", "PA130", "PA131", "PA132", "BC100", "BC105", "BC108", "GR106", diff --git a/demisto_sdk/commands/validate/initializer.py b/demisto_sdk/commands/validate/initializer.py index ba72d305aa..1609537d44 100644 --- a/demisto_sdk/commands/validate/initializer.py +++ b/demisto_sdk/commands/validate/initializer.py @@ -20,6 +20,7 @@ PLAYBOOKS_DIR, RELEASE_NOTES_DIR, SCRIPTS_DIR, + SKIP_RELEASE_NOTES_FOR_TYPES, GitStatuses, PathLevel, ) @@ -27,9 +28,12 @@ from demisto_sdk.commands.common.logger import logger from demisto_sdk.commands.common.tools import ( detect_file_level, + find_type, get_file_by_status, + get_pack_name, get_relative_path_from_packs_dir, is_external_repo, + pack_name_to_path, specify_files_from_directory, ) from demisto_sdk.commands.content_graph.objects.base_content import BaseContent @@ -299,6 +303,24 @@ def specify_files_by_status( return filtered_modified_files, filtered_added_files, filtered_renamed_files + @staticmethod + def get_associated_pack_metadata(content_files: Set[BaseContent]): + pack_names = [] + pack_metadatas: Set[Optional[BaseContent]] = set() + for content_file in content_files: + if find_type(str(content_file.path)) not in SKIP_RELEASE_NOTES_FOR_TYPES: + pack_name = get_pack_name(content_file.path) + pack_path = pack_name_to_path(pack_name) + pack_names.append(pack_name) + pack_metadata = BaseContent.from_path( + Path(pack_path) / PACKS_PACK_META_FILE_NAME, metadata_only=True + ) + pack_metadatas.add(pack_metadata) + + if pack_metadatas: + logger.info(f"Running on pack metadata files in packs: {pack_names}") + return pack_metadatas + def gather_objects_to_run_on( self, ) -> Tuple[Set[BaseContent], Set[Path]]: @@ -317,6 +339,9 @@ def gather_objects_to_run_on( invalid_content_items, non_content_items, ) = self.get_files_using_git() + content_objects_to_run = content_objects_to_run.union( + self.get_associated_pack_metadata(content_objects_to_run) + ) elif self.file_path: ( content_objects_to_run, @@ -339,6 +364,9 @@ def gather_objects_to_run_on( invalid_content_items, non_content_items, ) = self.get_files_using_git() + content_objects_to_run = content_objects_to_run.union( + self.get_associated_pack_metadata(content_objects_to_run) + ) if not self.use_git: content_objects_to_run_with_packs: Set[ BaseContent diff --git a/demisto_sdk/commands/validate/sdk_validation_config.toml b/demisto_sdk/commands/validate/sdk_validation_config.toml index bb7caad073..476ee23c79 100644 --- a/demisto_sdk/commands/validate/sdk_validation_config.toml +++ b/demisto_sdk/commands/validate/sdk_validation_config.toml @@ -23,7 +23,7 @@ ignorable_errors = [ [use_git] select = [ "BA100", "BA101", "BA105", "BA106", "BA108", "BA109", "BA110", "BA111", "BA114", "BA116", "BA118", "BA119", "BA124", "BA125", "BA126", - "PA100", "PA101", "PA102", "PA103", "PA104", "PA105", "PA107", "PA108", "PA109", "PA111", "PA113", "PA115", "PA117", "PA118", "PA119", "PA120", + "PA100", "PA101", "PA102", "PA103", "PA104", "PA105", "PA107", "PA108", "PA109", "PA111", "PA113", "PA114", "PA115", "PA117", "PA118", "PA119", "PA120", "PA121", "PA123", "PA125", "PA127", "PA128", "PA130", "PA131", "PA132", "BC100", "BC101", "BC102", "BC103", "BC104", "BC105", "BC108", "BC110", "BC111", "IN100", "IN101", "IN102", "IN104", "IN106", "IN107", "IN108", "IN109", "IN110", "IN112", "IN113", "IN114", "IN115", "IN117", "IN118", "IN121", "IN122", "IN123", "IN124", "IN125", "IN126", "IN127", "IN130", diff --git a/demisto_sdk/commands/validate/tests/PA_validators_test.py b/demisto_sdk/commands/validate/tests/PA_validators_test.py index 16dccfb47d..d53f1e3b18 100644 --- a/demisto_sdk/commands/validate/tests/PA_validators_test.py +++ b/demisto_sdk/commands/validate/tests/PA_validators_test.py @@ -56,6 +56,9 @@ from demisto_sdk.commands.validate.validators.PA_validators.PA113_is_url_or_email_exists import ( IsURLOrEmailExistsValidator, ) +from demisto_sdk.commands.validate.validators.PA_validators.PA114_pack_metadata_version_should_be_raised import ( + PackMetadataVersionShouldBeRaisedValidator, +) from demisto_sdk.commands.validate.validators.PA_validators.PA115_is_created_field_in_iso_format import ( IsCreatedFieldInISOFormatValidator, ) @@ -1613,3 +1616,32 @@ def test_PackFilesValidator_fix(file_attribute: str): assert meta_file.file_path.exists() assert meta_file.exist # changed in the fix + + +@pytest.mark.parametrize( + "old_version, current_version, expected_invalid", + [("1.0.0", "1.0.1", 0), ("1.0.0", "1.0.0", 1), ("1.1.0", "1.0.1", 1)], +) +def test_PackMetadataVersionShouldBeRaisedValidator( + mocker, old_version, current_version, expected_invalid +): + """ + Given: A previous pack version and a current pack version. + When: Running PackMetadataVersionShouldBeRaisedValidator validator. + Then: Assure the validation succeeds if the current version <= previous version. + Cases: + 1) current version > previous version: 0 validation errors. + 2) current version = previous version: 1 validation errors. + 3) current version < previous version: 1 validation errors. + """ + pack = create_pack_object(["currentVersion"], [current_version]) + integration = create_integration_object() + pack.content_items.integration.extend(integration) + mocker.patch( + "demisto_sdk.commands.validate.validators.PA_validators" + ".PA114_pack_metadata_version_should_be_raised.get_remote_file", + return_value={"currentVersion": old_version}, + ) + version_bump_validator = PackMetadataVersionShouldBeRaisedValidator() + version_bump_validator.external_args["prev_ver"] = "origin/master" + assert len(version_bump_validator.is_valid([pack])) == expected_invalid diff --git a/demisto_sdk/commands/validate/tests/validators_test.py b/demisto_sdk/commands/validate/tests/validators_test.py index dc10248a42..8cb3f9a18d 100644 --- a/demisto_sdk/commands/validate/tests/validators_test.py +++ b/demisto_sdk/commands/validate/tests/validators_test.py @@ -12,13 +12,17 @@ from demisto_sdk.commands.common.content_constant_paths import CONTENT_PATH from demisto_sdk.commands.common.handlers import DEFAULT_JSON_HANDLER as json from demisto_sdk.commands.content_graph.common import ContentType +from demisto_sdk.commands.content_graph.objects.base_content import BaseContent from demisto_sdk.commands.content_graph.tests.test_tools import load_yaml from demisto_sdk.commands.validate.config_reader import ( ConfigReader, ConfiguredValidations, ) from demisto_sdk.commands.validate.initializer import Initializer -from demisto_sdk.commands.validate.tests.test_tools import create_integration_object +from demisto_sdk.commands.validate.tests.test_tools import ( + create_integration_object, + create_pack_object, +) from demisto_sdk.commands.validate.validate_manager import ValidateManager from demisto_sdk.commands.validate.validation_results import ResultWriter from demisto_sdk.commands.validate.validators.BA_validators.BA101_id_should_equal_name import ( @@ -40,6 +44,9 @@ from demisto_sdk.commands.validate.validators.PA_validators.PA108_pack_metadata_name_not_valid import ( PackMetadataNameValidator, ) +from demisto_sdk.commands.validate.validators.PA_validators.PA114_pack_metadata_version_should_be_raised import ( + PackMetadataVersionShouldBeRaisedValidator, +) from TestSuite.test_tools import str_in_call_args_list INTEGRATION = create_integration_object() @@ -655,3 +662,44 @@ def test_description(): assert not [ validator for validator in get_all_validators() if not validator.description ] + + +def test_check_metadata_version_bump_on_content_changes(mocker, repo): + """ + Given: pack with newly added integration. + When: Initializing ValidateManager using git + Then: Ensure PackMetadataVersionShouldBeRaisedValidator is initialized and the external args are properly passed. + """ + pack = create_pack_object(["currentVersion"], ["1.1.1"]) + integration = create_integration_object() + pack.content_items.integration.extend(integration) + validation_results = ResultWriter() + config_reader = ConfigReader(category_to_run="use_git") + mocker.patch.object( + Initializer, + "gather_objects_to_run_on", + return_value=( + { + BaseContent.from_path(Path(integration.path)), + BaseContent.from_path(Path(pack.path)), + }, + {}, + ), + ) + initializer = Initializer(prev_ver="some_prev_ver") + + validate_manager = ValidateManager( + validation_results=validation_results, + config_reader=config_reader, + initializer=initializer, + ) + + version_bump_validator = None + for validator in validate_manager.validators: + if isinstance(validator, PackMetadataVersionShouldBeRaisedValidator): + version_bump_validator = validator + + # Assert the PA114 validation will run + assert version_bump_validator + # Assert external args were propagated correctly. + assert version_bump_validator.external_args["prev_ver"] == "some_prev_ver" diff --git a/demisto_sdk/commands/validate/validate_manager.py b/demisto_sdk/commands/validate/validate_manager.py index d1cbcca0e8..69a59ed407 100644 --- a/demisto_sdk/commands/validate/validate_manager.py +++ b/demisto_sdk/commands/validate/validate_manager.py @@ -52,6 +52,7 @@ def __init__( ) ) self.validators = self.filter_validators() + self.update_validators_external_args() def run_validations(self) -> int: """ @@ -122,6 +123,14 @@ def filter_validators(self) -> List[BaseValidator]: if validator.error_code in self.configured_validations.validations_to_run ] + def update_validators_external_args(self): + """Update the validators with args from the initializer.""" + for validator in self.validators: + if validator.external_args: + for external_arg_name in validator.external_args.keys(): + external_arg_value = getattr(self.initializer, external_arg_name) + validator.external_args[external_arg_name] = external_arg_value + def add_invalid_content_items(self): """Create results for all the invalid_content_items. diff --git a/demisto_sdk/commands/validate/validators/PA_validators/PA114_pack_metadata_version_should_be_raised.py b/demisto_sdk/commands/validate/validators/PA_validators/PA114_pack_metadata_version_should_be_raised.py new file mode 100644 index 0000000000..324252e54a --- /dev/null +++ b/demisto_sdk/commands/validate/validators/PA_validators/PA114_pack_metadata_version_should_be_raised.py @@ -0,0 +1,61 @@ +from __future__ import annotations + +from typing import Iterable, List + +from packaging.version import Version + +from demisto_sdk.commands.common.constants import ( + DEMISTO_GIT_PRIMARY_BRANCH, + PACKS_PACK_META_FILE_NAME, +) +from demisto_sdk.commands.common.tools import get_remote_file +from demisto_sdk.commands.content_graph.objects.pack import Pack +from demisto_sdk.commands.validate.validators.base_validator import ( + BaseValidator, + ValidationResult, +) + +ContentTypes = Pack + + +class PackMetadataVersionShouldBeRaisedValidator(BaseValidator[ContentTypes]): + error_code = "PA114" + description = "Ensure that the pack metadata is raised on relevant changes." + rationale = ( + "When updating a pack, its version needs to be raised to maintain traceability." + ) + error_message = ( + "The pack version (currently: {old_version}) needs to be raised - " + "make sure you are merged from master and " + 'update the "currentVersion" field in the ' + "pack_metadata.json or in case release notes are required run:\n" + "`demisto-sdk update-release-notes -i Packs/{pack} -u " + "(major|minor|revision|documentation)` to " + "generate them according to the new standard." + ) + related_field = "currentVersion, name" + is_auto_fixable = False + # Validate manager should populate the external args for the validation. + external_args = {"prev_ver": None} + + def is_valid(self, content_items: Iterable[ContentTypes]) -> List[ValidationResult]: + validation_results = [] + prev_ver_tag = self.external_args.get("prev_ver", DEMISTO_GIT_PRIMARY_BRANCH) + for content_item in content_items: + old_meta_file_content = get_remote_file( + str(content_item.path / PACKS_PACK_META_FILE_NAME), tag=prev_ver_tag + ) + old_version = old_meta_file_content.get("currentVersion", "0.0.0") + if content_item.current_version and Version(old_version) >= Version( + content_item.current_version + ): + validation_results.append( + ValidationResult( + validator=self, + message=self.error_message.format( + old_version=old_version, pack=content_item.name + ), + content_object=content_item, + ) + ) + return validation_results diff --git a/demisto_sdk/commands/validate/validators/base_validator.py b/demisto_sdk/commands/validate/validators/base_validator.py index 42e9bfd822..2d5c88b0aa 100644 --- a/demisto_sdk/commands/validate/validators/base_validator.py +++ b/demisto_sdk/commands/validate/validators/base_validator.py @@ -95,6 +95,7 @@ class BaseValidator(ABC, BaseModel, Generic[ContentTypes]): is_auto_fixable: ClassVar[bool] = False graph_interface: ClassVar[ContentGraphInterface] = None related_file_type: ClassVar[Optional[List[RelatedFileType]]] = None + external_args: ClassVar[dict] = {} def get_content_types(self): args = (get_args(self.__orig_bases__[0]) or get_args(self.__orig_bases__[1]))[0] # type: ignore From 19ecd112ed618d29241e33c7ff55c4ec9e7dfb35 Mon Sep 17 00:00:00 2001 From: dmeylakh Date: Thu, 13 Jun 2024 19:31:42 +0300 Subject: [PATCH 2/5] comitting changelog --- .changelog/4342.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 .changelog/4342.yml diff --git a/.changelog/4342.yml b/.changelog/4342.yml new file mode 100644 index 0000000000..122977e548 --- /dev/null +++ b/.changelog/4342.yml @@ -0,0 +1,4 @@ +changes: +- description: Added the PA114 validation to the new **validate** command. + type: internal +pr_number: 4342 \ No newline at end of file From 0e2c9654f6877b3f8575e4476d67530b6f51377c Mon Sep 17 00:00:00 2001 From: dmeylakh Date: Thu, 13 Jun 2024 19:39:47 +0300 Subject: [PATCH 3/5] comitting changelog --- .changelog/4342.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/4342.yml b/.changelog/4342.yml index 122977e548..f21861db40 100644 --- a/.changelog/4342.yml +++ b/.changelog/4342.yml @@ -1,4 +1,4 @@ changes: - description: Added the PA114 validation to the new **validate** command. type: internal -pr_number: 4342 \ No newline at end of file +pr_number: 4342 From 52590e09938047677d9444c22a7749f171773efa Mon Sep 17 00:00:00 2001 From: dmeylakh Date: Thu, 20 Jun 2024 10:09:30 +0300 Subject: [PATCH 4/5] fix types and tests and such --- demisto_sdk/commands/validate/initializer.py | 17 ++++++++++++++--- .../validate/sdk_validation_config.toml | 2 +- .../commands/validate/tests/validators_test.py | 15 +++++++-------- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/demisto_sdk/commands/validate/initializer.py b/demisto_sdk/commands/validate/initializer.py index 1609537d44..193453e2dd 100644 --- a/demisto_sdk/commands/validate/initializer.py +++ b/demisto_sdk/commands/validate/initializer.py @@ -304,9 +304,19 @@ def specify_files_by_status( return filtered_modified_files, filtered_added_files, filtered_renamed_files @staticmethod - def get_associated_pack_metadata(content_files: Set[BaseContent]): + def get_associated_pack_metadata( + content_files: Set[BaseContent], + ) -> Set[BaseContent]: + """Get associated pack metadata files even if they were not changed/given explicitly. + + Args: + content_files (Set[BaseContent]): The files to get their metadata pairs. + + Return: + (Set[BaseContent]): A set of the relevant pack metadata files. + """ pack_names = [] - pack_metadatas: Set[Optional[BaseContent]] = set() + pack_metadatas: Set[BaseContent] = set() for content_file in content_files: if find_type(str(content_file.path)) not in SKIP_RELEASE_NOTES_FOR_TYPES: pack_name = get_pack_name(content_file.path) @@ -315,7 +325,8 @@ def get_associated_pack_metadata(content_files: Set[BaseContent]): pack_metadata = BaseContent.from_path( Path(pack_path) / PACKS_PACK_META_FILE_NAME, metadata_only=True ) - pack_metadatas.add(pack_metadata) + if pack_metadata: + pack_metadatas.add(pack_metadata) if pack_metadatas: logger.info(f"Running on pack metadata files in packs: {pack_names}") diff --git a/demisto_sdk/commands/validate/sdk_validation_config.toml b/demisto_sdk/commands/validate/sdk_validation_config.toml index d86d3ce462..bdba0c0726 100644 --- a/demisto_sdk/commands/validate/sdk_validation_config.toml +++ b/demisto_sdk/commands/validate/sdk_validation_config.toml @@ -5,7 +5,7 @@ ignorable_errors = [ "IF100", "IF106", "IF113", "IF115", "IF116", "IN107", "IN109", "IN110", "IN122", "IN123", "IN126", "IN128", "IN135", "IN136", "IN139", "IN144", "IN145", "IN150", "IN153", "IN154", "IN161", "MP106", - "PA113", "PA116", "PA124", "PA125", "PA127", "PA129", + "PA113", "PA114", "PA116", "PA124", "PA125", "PA127", "PA129", "PB104", "PB105", "PB106", "PB107", "PB110", "PB111", "PB114", "PB115", "PB116", "PB118", "PB119", "PB121", "RM100", "RM102", "RM104", "RM106", "RM110", "RM112", "RM113", "RP102", "RP104", diff --git a/demisto_sdk/commands/validate/tests/validators_test.py b/demisto_sdk/commands/validate/tests/validators_test.py index 8cb3f9a18d..d8b5e1de98 100644 --- a/demisto_sdk/commands/validate/tests/validators_test.py +++ b/demisto_sdk/commands/validate/tests/validators_test.py @@ -677,14 +677,13 @@ def test_check_metadata_version_bump_on_content_changes(mocker, repo): config_reader = ConfigReader(category_to_run="use_git") mocker.patch.object( Initializer, - "gather_objects_to_run_on", - return_value=( - { - BaseContent.from_path(Path(integration.path)), - BaseContent.from_path(Path(pack.path)), - }, - {}, - ), + "get_files_using_git", + return_value=({BaseContent.from_path(Path(integration.path))}, {}, {}), + ) + mocker.patch.object( + BaseContent, + "from_path", + return_value=BaseContent.from_path(Path(pack.path), metadata_only=True), ) initializer = Initializer(prev_ver="some_prev_ver") From 04778cf8389f2b6b8a3e543de731df6bfb9529f1 Mon Sep 17 00:00:00 2001 From: dmeylakh Date: Thu, 20 Jun 2024 11:08:35 +0300 Subject: [PATCH 5/5] recover skip pack dependencies flag --- demisto_sdk/__main__.py | 4 ++-- demisto_sdk/commands/validate/initializer.py | 9 ++++++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/demisto_sdk/__main__.py b/demisto_sdk/__main__.py index 4de7988cff..1596363ca8 100644 --- a/demisto_sdk/__main__.py +++ b/demisto_sdk/__main__.py @@ -705,7 +705,7 @@ def zip_packs(ctx, **kwargs) -> int: @click.option( "--skip-pack-dependencies", is_flag=True, - help="Relevant only for the old validate flow and will be removed in a future release. Skip validation of pack dependencies.", + help="Skip validation of pack dependencies.", ) @click.option( "--create-id-set", @@ -856,7 +856,6 @@ def validate(ctx, config, file_paths: str, **kwargs): "print_ignored_files", "no_docker_checks", "silence_init_prints", - "skip_pack_dependencies", "id_set_path", "create_id_set", "skip_schema_check", @@ -920,6 +919,7 @@ def validate(ctx, config, file_paths: str, **kwargs): prev_ver=kwargs["prev_ver"], file_path=file_path, all_files=kwargs.get("validate_all"), + skip_pack_dependencies=kwargs.get("skip_pack_dependencies"), ) validator_v2 = ValidateManager( file_path=file_path, diff --git a/demisto_sdk/commands/validate/initializer.py b/demisto_sdk/commands/validate/initializer.py index 193453e2dd..6c9cfdc2ba 100644 --- a/demisto_sdk/commands/validate/initializer.py +++ b/demisto_sdk/commands/validate/initializer.py @@ -60,6 +60,7 @@ def __init__( prev_ver=None, file_path=None, all_files=False, + skip_pack_dependencies=False, ): self.staged = staged self.use_git = use_git @@ -67,6 +68,7 @@ def __init__( self.all_files = all_files self.committed_only = committed_only self.prev_ver = prev_ver + self.skip_pack_dependencies = skip_pack_dependencies def validate_git_installed(self): """Initialize git util.""" @@ -303,8 +305,8 @@ def specify_files_by_status( return filtered_modified_files, filtered_added_files, filtered_renamed_files - @staticmethod def get_associated_pack_metadata( + self, content_files: Set[BaseContent], ) -> Set[BaseContent]: """Get associated pack metadata files even if they were not changed/given explicitly. @@ -317,6 +319,11 @@ def get_associated_pack_metadata( """ pack_names = [] pack_metadatas: Set[BaseContent] = set() + + if self.skip_pack_dependencies: + logger.debug("Skipping pack metadata files validation.") + return set() + for content_file in content_files: if find_type(str(content_file.path)) not in SKIP_RELEASE_NOTES_FOR_TYPES: pack_name = get_pack_name(content_file.path)