Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Add optional switch for disabling custom git fetch#42704

Merged
evict merged 7 commits into
mainfrom
vincent/optional-disable-custom-git-fetch
Oct 14, 2022
Merged

Add optional switch for disabling custom git fetch#42704
evict merged 7 commits into
mainfrom
vincent/optional-disable-custom-git-fetch

Conversation

@evict

@evict evict commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

Disable custom git fetch through an environment variable

Test plan

  • ci tests
  • manual review

@cla-bot cla-bot Bot added the cla-signed label Oct 7, 2022
@evict evict requested review from a team and daxmc99 October 7, 2022 14:54
@sourcegraph-bot

sourcegraph-bot commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 7b7e3a9...8c1ced0.

Notify File(s)
@indradhanush cmd/gitserver/server/customfetch.go
cmd/gitserver/server/customfetch_test.go
@ryanslade cmd/gitserver/server/customfetch.go
cmd/gitserver/server/customfetch_test.go
@sashaostrikov cmd/gitserver/server/customfetch.go
cmd/gitserver/server/customfetch_test.go

Comment thread cmd/gitserver/server/customfetch.go Outdated

@daxmc99 daxmc99 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This also needs to be mentioned in our CHANGELOG.md file

@DaedalusG DaedalusG left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a much better approach than mounting a json file with the repo to fetch mapping 👍
I do agree with Dax, probably better to have it disabled by default. Branched and switched the booleans -- PR: https://github.com/sourcegraph/sourcegraph/pull/42723

@evict evict requested review from DaedalusG and daxmc99 October 10, 2022 12:56

@DaedalusG DaedalusG left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this has been tested manually, I think it should be good to ship 👍

@daxmc99 daxmc99 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, I'll defer to @DaedalusG to get customer feedback on this. We should inform them of this change prior to the release so they aren't blindsided

Comment thread cmd/gitserver/server/customfetch_test.go
Comment thread cmd/gitserver/server/customfetch.go
@evict evict force-pushed the vincent/optional-disable-custom-git-fetch branch from e3292f4 to 8e2d196 Compare October 12, 2022 09:14

@daxmc99 daxmc99 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! This approach makes sense.
One last note is that the large diff here seems to be due to some auto-formatting running over the JSON. Might want to regen the schema again to avoid this.

Vincent Ruijter and others added 7 commits October 14, 2022 10:19
@evict evict force-pushed the vincent/optional-disable-custom-git-fetch branch from d833d5c to 8c1ced0 Compare October 14, 2022 08:21
@evict evict merged commit e92afd5 into main Oct 14, 2022
@evict evict deleted the vincent/optional-disable-custom-git-fetch branch October 14, 2022 09:06
@evict evict added the SSDLC label Oct 17, 2022

@amieroth amieroth left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All good on my side.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants