Skip to content

Conversation

kax-slamcore
Copy link

@kax-slamcore kax-slamcore commented May 5, 2022

Fix retryFetch and clobberOnFailure.

As per http://docs.buildbot.net/latest/manual/configuration/steps/source_git.html#

retryFetch (optional, default: False)

If true, if the git fetch fails, then Buildbot retries to fetch again instead of failing the entire source checkout.

However, this does not currently work.

In Git._fetchOrFallback, the call to self._fetch(None) will throw a BuildStepFailed if the fetch command fails. The fallback code in _fetchOrFallback will not be executed and the fetch will not be retried. Same for clobberOnFailure=True if it is requested.

This change modifies _fetch to accept an abandonOnFailure flag to control whether it should throw.

Test Plan

In the first commit I've added tests to test_source_git.py for the retryFetch=True case that fail, demonstrating the issue.
The second commit fixes the failing tests.

Contributor Checklist:

  • I have updated the unit tests
  • I have created a file in the newsfragments directory (and read the README.txt in that directory)
  • I have updated the appropriate documentation - I will seek guidance.

@kax-slamcore kax-slamcore force-pushed the QA-269-retry-fetch-fails branch from 20d710d to df5f9bf Compare May 5, 2022 19:03
ExpectListdir(dir='source')
.files(['.git'])
.exit(0),
ExpectShell(workdir='source',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I may have misunderstood how these tests are working, but why are we expecting an exit code of 1 for this?

Copy link
Author

Choose a reason for hiding this comment

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

It's not expecting the exit code.
ExpectShell configures a mock which expects the git fetch command and then imitates a non-zero exit code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok, that's handy!

Copy link
Collaborator

@nicolas-slamcore nicolas-slamcore left a comment

Choose a reason for hiding this comment

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

The change makes sense to me and the tests are fair enough. 👍

Note that the
`test_mode_full_copy_recursive_fetch_fail_retry_{fail,succeed}` test
both fail to execute an expected second attempt to git fetch, and the
`test_mode_full_copy_recursive_fetch_fail_clobberOnFailure` test fails
to clobber and clone the source directory:

```
$ trial buildbot.test.unit.steps.test_source_git.TestGit

... (etc) ...

    test_mode_full_copy_recursive ...                                      [OK]
    test_mode_full_copy_recursive_fetch_fail ...                           [OK]
    test_mode_full_copy_recursive_fetch_fail_clobberOnFailure ...       [ERROR]
    test_mode_full_copy_recursive_fetch_fail_retry_fail ...             [ERROR]
    test_mode_full_copy_recursive_fetch_fail_retry_succeed ...          [ERROR]
```
@kax-slamcore kax-slamcore force-pushed the QA-269-retry-fetch-fails branch from df5f9bf to 831c2b9 Compare July 11, 2022 12:46
…requested.

In `Git._fetchOrFallback`, the call to `self._fetch(None)` will throw a
`BuildStepFailed` if the fetch command fails. The fallback code in
`_fetchOrFallback` will not be executed and the fetch will not be
retried nor will `clobber()` be called.

This change modifies `_fetch` to accept an `abandonOnFailure` flag to
control whether it should throw.

Test Plan

Run the unit tests added in the previous commit.
@kax-slamcore kax-slamcore force-pushed the QA-269-retry-fetch-fails branch from 831c2b9 to 7642e8c Compare July 11, 2022 12:53
@kax-slamcore kax-slamcore merged commit 7642e8c into master Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants