Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

demisto/etc#23233 added all files validation #343

Merged
merged 1 commit into from
Apr 21, 2020
Merged

demisto/etc#23233 added all files validation #343

merged 1 commit into from
Apr 21, 2020

Conversation

hod-alpert
Copy link
Contributor

Status

  • In Progress
  • Ready
  • In Hold - (Reason for hold)

Related Issues

fixes: demisto/etc#23233

Description

Adds an option to add full validation to all files using demisto-sdk validate -a

Must have

  • Tests
  • Documentation

@hod-alpert hod-alpert requested a review from Itay4 April 12, 2020 10:24
Itay4
Itay4 reviewed Apr 12, 2020
View reviewed changes
demisto_sdk/__main__.py Outdated
@@ -204,6 +204,9 @@ def unify(**kwargs):
@click.option(
'-p', '--path', help='Path of file to validate specifically, outside of a git directory.'
)
@click.option(
'-a', '--validate-all', is_flag=True, show_default=True, default=False, help='Whether to validate all files or not'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets improve description here, and add to it that it will run all validations

demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
@Itay4 Itay4 requested a review from yaakovi April 12, 2020 10:59
Itay4
Itay4 reviewed Apr 12, 2020
View reviewed changes
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
yaakovi
yaakovi previously requested changes Apr 12, 2020
View reviewed changes
Copy link
Contributor

@yaakovi yaakovi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run the command against content and make sure you catch all json/yml files.
there might be some paths missing in the regex coverage

demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py
if not structure_validator.is_valid_file():
self._is_valid = False

elif re.match(PLAYBOOK_REGEX, file_path, re.IGNORECASE) or file_type == 'playbook':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about pack/beta playbooks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beta playbooks will be removed to legacy playbooks

Packs playbooks are validated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where are they validated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this catches all files in Packs recursively

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then you get here and ignore them since they are not accepted by this regex

Copy link
Contributor Author

@hod-alpert hod-alpert Apr 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The playbooks are not ignored because of the or file_type == 'playbook' part

demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Apr 13, 2020

This pull request introduces 3 alerts when merging 0b80dbcdf110b971088f13f7490b297940e1526a into ccf04e0 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Unnecessary pass

yaakovi
yaakovi reviewed Apr 13, 2020
View reviewed changes
demisto_sdk/commands/common/hook_validations/image.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/file_validator.py Outdated Show resolved Hide resolved
demisto_sdk/commands/common/constants.py Outdated Show resolved Hide resolved
@Itay4 Itay4 mentioned this pull request Apr 14, 2020
Merged
3 tasks
@hod-alpert hod-alpert force-pushed the hod_validate_files branch 3 times, most recently from d0459a9 to dee6ec6 Compare April 16, 2020 07:28
@coveralls
Copy link
Collaborator

coveralls commented Apr 16, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 2573

  • 53 of 94 (56.38%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 66.2%
Changes Missing Coverage Covered Lines Changed/Added Lines %
demisto_sdk/commands/common/tools.py 6 7 85.71%
demisto_sdk/commands/validate/file_validator.py 45 85 52.94%
Totals Coverage Status
Change from base Build 2547: 0.1%
Covered Lines: 5104
Relevant Lines: 7710
💛 - Coveralls

@hod-alpert hod-alpert force-pushed the hod_validate_files branch 9 times, most recently from 134cb9c to a2e64f6 Compare April 20, 2020 07:57
@hod-alpert hod-alpert requested a review from Itay4 April 20, 2020 08:03
@hod-alpert hod-alpert requested a review from Itay4 April 20, 2020 13:42
@hod-alpert hod-alpert force-pushed the hod_validate_files branch 2 times, most recently from 8837967 to 14f021e Compare April 20, 2020 14:56
@lgtm-com
Copy link

lgtm-com bot commented Apr 20, 2020

This pull request introduces 1 alert when merging 14f021e95c7ad7ab6d7e9f43f595474e3d32ae91 into 1df3412 - view on LGTM.com

new alerts:

  • 1 for Unused import

@hod-alpert hod-alpert requested a review from yaakovi April 20, 2020 15:53
@hod-alpert hod-alpert dismissed yaakovi’s stale review April 21, 2020 07:15

Talked in person

@hod-alpert hod-alpert merged commit 6ff1b7c into master Apr 21, 2020
@hod-alpert hod-alpert deleted the hod_validate_files branch April 21, 2020 07:15
@guyfreund guyfreund mentioned this pull request Jun 2, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Itay4 Itay4 Itay4 approved these changes

@yaakovi yaakovi Awaiting requested review from yaakovi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@hod-alpert @coveralls @yaakovi @Itay4