[Pre-Commit] Continue Even if Fetch Failed

[MichaelYochpaz]

Related Issues

fixes: https://jira-dc.paloaltonetworks.com/browse/CIAC-9231

Description

Fix an issue (visible mainly on pre-commit) where if fetching from git fails due to connection problems, the whole command fails, even though it should be able to continue (just won't run fully up-to-date)

[GuyAfik]

@MichaelYochpaz does it fail because of connection errors? if yes lets use the retry decorator (you can find it in tools.py)

[coveralls]

Pull Request Test Coverage Report for Build 4331656b-3878-4fbb-8ccd-6e87677beba6

• 3 of 6 (50.0%) changed or added relevant lines in 1 file are covered.
• 8023 unchanged lines in 291 files lost coverage.
• Overall coverage decreased (-18.3%) to 80.583%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
demisto_sdk/commands/common/git_util.py	3	6	50.0%

Files with Coverage Reduction	New Missed Lines	%
demisto_sdk/commands/common/clients/configs.py	1	97.14%
demisto_sdk/commands/common/content/objects/pack_objects/incident_field/incident_field.py	1	65.0%
demisto_sdk/commands/common/content/objects/pack_objects/job/job.py	1	83.33%
demisto_sdk/commands/common/content/objects/pack_objects/lists/lists.py	1	85.71%
demisto_sdk/commands/common/files/errors.py	1	71.43%
demisto_sdk/commands/common/files/text_file.py	1	78.13%
demisto_sdk/commands/common/hook_validations/deprecation.py	1	98.72%
demisto_sdk/commands/common/hook_validations/job.py	1	98.41%
demisto_sdk/commands/common/hook_validations/layout_rule.py	1	88.89%
demisto_sdk/commands/common/hook_validations/lists.py	1	96.55%

Totals
Change from base Build 7694890770:	-18.3%
Covered Lines:	35762
Relevant Lines:	44379

---

💛 - Coveralls

[GuyAfik]

@MichaelYochpaz see my comment here

@ilaner is there a reason not to continue if fetch fails?

[MichaelYochpaz]

@MichaelYochpaz does it fail because of connection errors? if yes lets use the retry decorator (you can find it in tools.py)
This PR is for a case where there is no connection to GitHub for some reason (like firewall issues), to allow the pre-commit flow to continue even without fetching.

@GuyAfik Adding a retry mechanism for a different case (where it's an issue that might be fixed on a retry), is a good idea, but handles a different case.

Current code handles a case where there is no connection at all (like firewall issues, where retrying won't help).
But we can do retries first, and then if they all failed, skip the fetch. Will probably be better than the current code with no retries.

However, it's going to be problematic as the retry wrapper is in the tools module, which imports the git_util module that contains the fetch method. So this will cause circular imports issues.
The updated code would look like:

class GitUtil:

    @retry(times=3, exceptions=(ConnectionError, Timeout))
    @lru_cache
    def fetch(self):
        self.repo.remote().fetch()
...

We can create a new module (that doesn't have any internal imports, like constants and logger) and move the retry wrapper there if you think that's a good idea.

[ilaner]

good job