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

fix race and updating local mounted repositories #5937

Merged
merged 5 commits into from
Oct 24, 2022

Conversation

jakecoffman
Copy link
Member

@jakecoffman jakecoffman commented Oct 20, 2022

Currently the Updater:

  1. Gets the base commit
  2. Fetches manifest files
  3. Clones the repo (for ecosystems that clone)

This seems like a race. To get the base commit it makes a call to get the default branch (if one isn't specified) and then another call to get the HEAD commit, then it clones. That leaves some space where a user could have pushed up a change, and so the base commit and cloned repo will be different.

It makes more sense to me to clone, then get the base commit and fetch the files from the locally cloned repo.

This also fixes the issue that we have when testing with local repositories with the Dependabot CLI. We were having to pass the vendor_dependencies flag to trick Dependabot into using the local repo. Now with already_cloned? Dependabot can tell it can use the local repo.

@jakecoffman jakecoffman force-pushed the jakecoffman/fix-race-and-testing-local branch from f24b9e1 to a1a20e0 Compare October 20, 2022 15:44
@jakecoffman jakecoffman force-pushed the jakecoffman/fix-race-and-testing-local branch from a1a20e0 to e2fae45 Compare October 24, 2022 13:46
@jakecoffman jakecoffman marked this pull request as ready for review October 24, 2022 14:17
@jakecoffman jakecoffman requested a review from a team as a code owner October 24, 2022 14:17
Copy link
Member

@bdragon bdragon left a comment

Choose a reason for hiding this comment

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

Makes sense to me 👍

When deploying this I found a new error pop up caused by trying to get the base_commit_sha after the clone failed because the branch doesn't exist. We report the SHA based on the branch, and so if the clone failed then we can't really tell what branch we're on. So to prevent from hiding the root cause I added this check to see if this was really a git repo before trying to use it.
@jakecoffman
Copy link
Member Author

jakecoffman commented Oct 24, 2022

When deploying this I found a new error pop up caused by trying to get the base_commit_sha after the clone failed because the branch doesn't exist. Some users have a branch that does not exist in their dependabot.yml.

We report the SHA based on the branch, and so if the clone failed then we can't really tell what branch we're on. So to prevent from hiding the root cause I added this check (aa50524) to see if this was really a git repo before trying to use it.

This is somewhat minor, the job used to fail and it fails now too, but at least it reports what the issue is more accurately.

@jakecoffman jakecoffman merged commit b49c206 into main Oct 24, 2022
@jakecoffman jakecoffman deleted the jakecoffman/fix-race-and-testing-local branch October 24, 2022 20:26
@pavera pavera mentioned this pull request Oct 31, 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.

None yet

3 participants