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

Metadata fetching: Don't fallback to git if not installed #6409

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

jeffwidman
Copy link
Member

This git fallback is being hit whenever there's a non-200 response code. However, if git isn't installed, this will result in throwing:

Errno::ENOENT with No such file or directory - git

So only fallback to git if installed.

@jeffwidman
Copy link
Member Author

Put this up as a draft for feedback because there are two different possible approaches... before merge, will need to remove one of them.

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Maybe it's simpler to rescue Errno::ENOENT directly from running git ls-remote? I don't think that error can be raised in any situation other than git not being in PATH.

Copy link
Member Author

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

annotating the changes

common/lib/dependabot/git_metadata_fetcher.rb Outdated Show resolved Hide resolved
common/lib/dependabot/git_metadata_fetcher.rb Outdated Show resolved Hide resolved
common/lib/dependabot/git_metadata_fetcher.rb Outdated Show resolved Hide resolved
common/lib/dependabot/git_metadata_fetcher.rb Outdated Show resolved Hide resolved
common/lib/dependabot/git_metadata_fetcher.rb Outdated Show resolved Hide resolved
@jeffwidman jeffwidman force-pushed the dont-fallback-when-git-not-installed branch 2 times, most recently from c0df633 to 0750981 Compare July 21, 2023 23:10
@jeffwidman
Copy link
Member Author

Switched to rescuing Errno::ENOENT as suggested by @deivid-rodriguez

This is ready for review.

I'm not sure:

  1. how to write a unit test, as not sure how to mock out the underlying git is missing?
  2. If this actually needs a unit test, as it should be fairly rare/low impact, we only saw this happen briefly internally when we experimented with using a different base container for running :dependabot:... my guess is most users who use :dependabot: open source will be running on systems that have git installed. But happy to add if reviewers think it's worth the time cost of figuring out how to do it...

@jeffwidman jeffwidman marked this pull request as ready for review July 21, 2023 23:14
@jeffwidman jeffwidman requested a review from a team as a code owner July 21, 2023 23:14
@jeffwidman jeffwidman force-pushed the dont-fallback-when-git-not-installed branch from 0750981 to e7dba12 Compare July 21, 2023 23:14
This git fallback is being hit whenever there's a non-`200` response
code. However, if `git` isn't installed, this will result in throwing:
```shell
Errno::ENOENT with No such file or directory - git
```

So catch the error and handle it.
@jeffwidman jeffwidman force-pushed the dont-fallback-when-git-not-installed branch from e7dba12 to 3ff8f1c Compare July 25, 2023 21:16
@jeffwidman jeffwidman enabled auto-merge (squash) July 25, 2023 21:16
@jeffwidman jeffwidman merged commit 015fb77 into main Jul 25, 2023
91 checks passed
@jeffwidman jeffwidman deleted the dont-fallback-when-git-not-installed branch July 25, 2023 21:28
brettfo pushed a commit to brettfo/dependabot-core that referenced this pull request Oct 11, 2023
…ot#6409)

This git fallback is being hit whenever there's a non-`200` response
code. However, if `git` isn't installed, this will result in throwing:
```shell
Errno::ENOENT with No such file or directory - git
```

So catch the error and handle it.
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.

None yet

2 participants