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

repo-updater: Exclude Phabricator when listing external services for sync #5101

Merged
merged 2 commits into from Aug 6, 2019

Conversation

@mrnugget
Copy link
Contributor

commented Aug 6, 2019

🎉 👯‍♂️ Co-authored with @tsenart 👯‍♂️ 🎉

A customer reported that syncing external services failed after
upgrading from 3.4 to 3.5 with the following error message:

no clone URL available for repo with id=XYZ

After investigating the issue we found out that with 3.5 we changed the
Phabricator source to construct clone URLs for every repo it lists. That
fails on the customer instance.

And that happens every time repo-updater syncs.

But since Phabricator is not a real external service and shouldn't be
used to sync repos and clone them, it shouldn't be synced when the
repo-updater Syncer runs.

This commit changes the existing behavior of ListExternalServices to
never return Phabricator external services, except when it's
specifically specified (which is what we need for the
gitolite-Phabricator-sync worker).

repo-updater: Exclude Phabricator when listing external services for …
…sync

A customer reported that syncing external services failed after
upgrading from 3.4 to 3.5 with the following error message:

    no clone URL available for repo with id=XYZ

After investigating the issue we found out that with 3.5 we changed the
Phabricator source to construct clone URLs for every repo it lists. That
fails on the customer instance.

And that happens every time repo-updater syncs.

But since Phabricator is not a _real_ external service and shouldn't be
used to sync repos and clone them, it shouldn't be synced when the
repo-updater Syncer runs.

This commit changes the existing behavior of `ListExternalServices` to
_never_ return Phabricator external services, except when it's
specifically specified (which is what we need for the
gitolite-Phabricator-sync worker).

@mrnugget mrnugget requested review from tsenart and keegancsmith Aug 6, 2019

@codecov

This comment has been minimized.

Copy link

commented Aug 6, 2019

Codecov Report

Merging #5101 into master will decrease coverage by 0.04%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #5101      +/-   ##
==========================================
- Coverage   46.32%   46.28%   -0.05%     
==========================================
  Files         731      732       +1     
  Lines       44872    44913      +41     
  Branches     2602     2613      +11     
==========================================
- Hits        20787    20786       -1     
- Misses      22073    22115      +42     
  Partials     2012     2012
Impacted Files Coverage Δ
cmd/repo-updater/repos/phabricator.go 53.43% <0%> (ø) ⬆️
cmd/repo-updater/repos/store.go 83.52% <100%> (+0.14%) ⬆️
cmd/repo-updater/repos/testing.go 81.06% <100%> (+0.07%) ⬆️
browser/src/shared/components/CodeViewToolbar.tsx 0% <0%> (-7.15%) ⬇️
shared/src/components/ResultContainer.tsx 71.87% <0%> (-6.25%) ⬇️
shared/src/components/CodeExcerpt.tsx 91.48% <0%> (-4.35%) ⬇️
shared/src/panel/views/FileLocations.tsx 74% <0%> (-4%) ⬇️
...ared/src/panel/views/HierarchicalLocationsView.tsx 73.68% <0%> (-2.64%) ⬇️
browser/src/libs/github/util.tsx 22.3% <0%> (-1.66%) ⬇️
shared/src/languages.ts 5.4% <0%> (-0.11%) ⬇️
... and 47 more

@mrnugget mrnugget merged commit acc38f0 into master Aug 6, 2019

2 checks passed

buildkite/sourcegraph Build #40684 passed (16 minutes, 20 seconds)
Details
percy/Sourcegraph Visual review automatically approved, no visual changes found.
Details

@mrnugget mrnugget deleted the core/exclude-phabricator-sync branch Aug 6, 2019

mrnugget added a commit that referenced this pull request Aug 6, 2019
repo-updater: Exclude Phabricator when listing external services for …
…sync (#5101)

* repo-updater: Exclude Phabricator when listing external services for sync

A customer reported that syncing external services failed after
upgrading from 3.4 to 3.5 with the following error message:

    no clone URL available for repo with id=XYZ

After investigating the issue we found out that with 3.5 we changed the
Phabricator source to construct clone URLs for every repo it lists. That
fails on the customer instance.

And that happens every time repo-updater syncs.

But since Phabricator is not a _real_ external service and shouldn't be
used to sync repos and clone them, it shouldn't be synced when the
repo-updater Syncer runs.

This commit changes the existing behavior of `ListExternalServices` to
_never_ return Phabricator external services, except when it's
specifically specified (which is what we need for the
gitolite-Phabricator-sync worker).

* repo-updater: Don't fail if Phabricator cloneURL could not be built
mrnugget added a commit that referenced this pull request Aug 6, 2019
repo-updater: Exclude Phabricator when listing external services for …
…sync (#5101)

* repo-updater: Exclude Phabricator when listing external services for sync

A customer reported that syncing external services failed after
upgrading from 3.4 to 3.5 with the following error message:

    no clone URL available for repo with id=XYZ

After investigating the issue we found out that with 3.5 we changed the
Phabricator source to construct clone URLs for every repo it lists. That
fails on the customer instance.

And that happens every time repo-updater syncs.

But since Phabricator is not a _real_ external service and shouldn't be
used to sync repos and clone them, it shouldn't be synced when the
repo-updater Syncer runs.

This commit changes the existing behavior of `ListExternalServices` to
_never_ return Phabricator external services, except when it's
specifically specified (which is what we need for the
gitolite-Phabricator-sync worker).

* repo-updater: Don't fail if Phabricator cloneURL could not be built
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.