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.

Sign up for GitHub

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

Jump to bottom

Add optional switch for disabling custom git fetch #42704

Merged
merged 7 commits into from Oct 14, 2022
Merged

Add optional switch for disabling custom git fetch #42704

merged 7 commits into from Oct 14, 2022

Conversation

evict
Copy link
Contributor

@evict evict commented Oct 7, 2022 •
edited

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 daxmc99 and a team October 7, 2022 14:54
@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Oct 7, 2022 •
edited

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

@evict evict mentioned this pull request Oct 7, 2022
Closed
@daxmc99 daxmc99 requested a review from DaedalusG October 7, 2022 20:11
daxmc99
daxmc99 requested changes Oct 7, 2022
View reviewed changes
Copy link
Member

@daxmc99 daxmc99 left a comment

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 requested changes Oct 7, 2022
View reviewed changes
Copy link
Contributor

@DaedalusG DaedalusG left a comment

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: #42723

DaedalusG
DaedalusG approved these changes Oct 10, 2022
View reviewed changes
Copy link
Contributor

@DaedalusG DaedalusG left a comment

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 approved these changes Oct 10, 2022
View reviewed changes
Copy link
Member

@daxmc99 daxmc99 left a comment

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

cmd/gitserver/server/customfetch_test.go Show resolved Hide resolved
@evict evict force-pushed the vincent/optional-disable-custom-git-fetch branch from e3292f4 to 8e2d196 Compare October 12, 2022 09:14
daxmc99
daxmc99 approved these changes Oct 13, 2022
View reviewed changes
Copy link
Member

@daxmc99 daxmc99 left a comment

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.

@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 reviewed Nov 22, 2022
View reviewed changes
Copy link

@amieroth amieroth left a comment

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 join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valerybugakov valerybugakov valerybugakov left review comments

@amieroth amieroth amieroth left review comments

@DaedalusG DaedalusG DaedalusG approved these changes

@daxmc99 daxmc99 daxmc99 approved these changes

Assignees
No one assigned
Labels
cla-signed SSDLC
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@evict @sourcegraph-bot @valerybugakov @DaedalusG @daxmc99 @amieroth