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.

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

validate that git is installed before executing any command. #3568

Merged
merged 46 commits into from
Sep 10, 2023

Conversation

GuyAfik
Copy link
Contributor

@GuyAfik GuyAfik commented Sep 3, 2023

Related Issues

fixes: https://jira-hq.paloaltonetworks.local/browse/XSUP-27651

Description

  1. validate that git is installed before executing any command and if not exit gracefully
  2. make sure that for git operations we will only use the GitUtil class and not Repo class to manage git operations anymore.
  3. add a ruff rule to prevent devs to import git.repo, but instead use GitUtil class
image

logger = logging.getLogger("demisto-sdk")
handle_deprecated_args(ctx.args)

config.configuration = Configuration()
import dotenv

# make sure that git is installed before using any command.
try:
subprocess.check_output(["git", "--version"])
Copy link
Contributor

Choose a reason for hiding this comment

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

See GitUtil, we (re)raise InvalidGitRepositoryError, better handle it there.

Copy link
Contributor Author

@GuyAfik GuyAfik Sep 3, 2023

Choose a reason for hiding this comment

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

however, we have several places that we just the Repo class from github-python and not the GitUtil object.
In order words we do not use only the GitUtil class for git stuff.

Copy link
Contributor Author

@GuyAfik GuyAfik Sep 4, 2023

Choose a reason for hiding this comment

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

Its not possible to do it in GitUtil as we import git module in __main__.py and if git is not installed, its not possible to import git at all, an ImportError exception will be raised.

The SDK crashes over it

Traceback (most recent call last):
  File "/usr/local/bin/demisto-sdk", line 5, in <module>
    from demisto_sdk.__main__ import main
  File "/usr/local/lib/python3.10/site-packages/demisto_sdk/__main__.py", line 11, in <module>
    import git
  File "/usr/local/lib/python3.10/site-packages/git/__init__.py", line 91, in <module>
    raise ImportError("Failed to initialize: {0}".format(_exc)) from _exc
ImportError: Failed to initialize: Bad git executable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

basically it crashes when the first git imported is reached (which happens in __main__.py)

@GuyAfik GuyAfik marked this pull request as ready for review September 5, 2023 07:57
@GuyAfik GuyAfik requested a review from dorschw September 5, 2023 07:57
Copy link
Contributor

@dorschw dorschw left a comment

Choose a reason for hiding this comment

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

Very nice

CHANGELOG.md Show resolved Hide resolved
demisto_sdk/__main__.py Show resolved Hide resolved
repo = None
if content_path := os.getenv("DEMISTO_SDK_CONTENT_PATH"):
git_util = GitUtil(Path(content_path))
logger.debug(f"Using content path: {content_path}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.debug(f"Using content path: {content_path}")

let's move the log line into the GitUtil constructor, f"GitUtil: using {content_path=}"

Copy link
Contributor Author

@GuyAfik GuyAfik Sep 5, 2023

Choose a reason for hiding this comment

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

can't use logger in GitUtil due to circular import :(
we have standard to use the logger from demisto_sdk.commands.common.logger?
or may i use the logger from logging module..
i prefer to keep our standards

demisto_sdk/commands/common/git_util.py Outdated Show resolved Hide resolved
demisto_sdk/commands/common/git_util.py Outdated Show resolved Hide resolved
demisto_sdk/commands/common/content/content.py Outdated Show resolved Hide resolved
@coveralls
Copy link
Collaborator

coveralls commented Sep 10, 2023

Pull Request Test Coverage Report for Build d8ebd8bc-7655-4401-8f31-5c017b0dfead

  • 38 of 44 (86.36%) changed or added relevant lines in 13 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.004%) to 81.167%

Changes Missing Coverage Covered Lines Changed/Added Lines %
demisto_sdk/commands/common/hook_validations/content_entity_validator.py 0 1 0.0%
demisto_sdk/commands/common/hook_validations/graph_validator.py 0 1 0.0%
demisto_sdk/commands/common/tools.py 6 7 85.71%
demisto_sdk/commands/create_artifacts/content_artifacts_creator.py 0 1 0.0%
demisto_sdk/commands/common/content/content.py 10 12 83.33%
Totals Coverage Status
Change from base Build dede1326-2d45-485f-ba20-3b64febbac6a: -0.004%
Covered Lines: 31967
Relevant Lines: 39384

💛 - Coveralls

@GuyAfik GuyAfik merged commit d0c24d2 into master Sep 10, 2023
18 checks passed
@GuyAfik GuyAfik deleted the no_git_custom_error_message branch September 10, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants