Require env var for custom git fetch #39630

daxmc99 · 2022-07-28T18:38:35Z

Move customGitFetch configuration from site-admin to a local file.

Test plan

Manual review
Manual testing

evict · 2022-08-04T14:06:47Z

Thanks @andreeleuterio for proposing to move it to a file! Makes things a lot easier.

sourcegraph-bot · 2022-08-04T14:25:55Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 91e8afb...b07359b.

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
@sourcegraph/delivery	doc/admin/how-to/customGitFetch.md doc/admin/how-to/index.md doc/admin/monorepo.md

cmd/gitserver/server/customfetch_test.go

unknwon

Thanks and LGTM other than some world-class formatting issues 😛

cmd/gitserver/server/customfetch.go

CHANGELOG.md

cmd/gitserver/server/customfetch_test.go

Co-authored-by: Joe Chen <joe@sourcegraph.com>

CHANGELOG.md

DaedalusG

Assuming the CUSTOM_GIT_FETCH_CONF env var is to be set on the sourcegraph-frontend deployment? Does this env var need to be set on any other deployments?

unknwon · 2022-08-17T07:48:49Z

Assuming the CUSTOM_GIT_FETCH_CONF env var is to be set on the sourcegraph-frontend deployment? Does this env var need to be set on any other deployments?

Should be set on all gitserver not sourcegraph-frontend I think? Defer to @sourcegraph/repo-management

Co-authored-by: Joe Chen <joe@sourcegraph.com>

DaedalusG · 2022-09-01T02:19:18Z

Moved the docs updates from a separate branch to this one 👍, please take one more look at this since I've created a dedicated usage doc @daxmc99 @unknwon

daxmc99 · 2022-09-01T17:00:09Z

@evict @andreeleuterio Is there a reason we moved this to a file?
I actually think this UX is noticeably harder for the critical customer who uses this.
I don't think this can be complete until we provide precise instructions for how to use this in a k8s deployment environment.
I'm happy to do that but I'm not fully understanding what moving from a config option to a file gives us.

It seems simpler to keep the config in the site-config and leverage an infrastructure-level config option to tune it.

DaedalusG

Holding this till we can provide easy instructions of how to mount the config file

keegancsmith

What is the original motivation for this? TBH what we are doing now is just more clunky and lives outside of things like being able to use the migrator/etc for it.

Generally when deprecating something we give a release or two before making it non functional.

cmd/gitserver/server/customfetch_test.go

schema/site.schema.json

Co-authored-by: Keegan Carruthers-Smith <keegan.csmith@gmail.com>

andreeleuterio · 2022-09-03T03:24:10Z

@keegancsmith you can find more context here. tl;dr is that this can't be a site-admin setting in the context of Managed Instances.

ryanslade · 2022-09-26T15:18:32Z

schema/site.schema.json

@@ -303,11 +303,11 @@
          }
        },
        "customGitFetch": {
-          "description": "JSON array of configuration that maps from Git clone URL domain/path to custom git fetch command.",
+          "description": "DEPRECATED: THIS VALUE WILL IS IGNORED. CONFIGURATION MOVED TO A LOCAL FILE USE ENVIRONMENT VARIABLE `CUSTOM_GIT_FETCH_CONF` TO SET FILE PATH. JSON array of configuration that maps from Git clone URL domain/path to custom git fetch command.",


Suggested change

"description": "DEPRECATED: THIS VALUE WILL IS IGNORED. CONFIGURATION MOVED TO A LOCAL FILE USE ENVIRONMENT VARIABLE `CUSTOM_GIT_FETCH_CONF` TO SET FILE PATH. JSON array of configuration that maps from Git clone URL domain/path to custom git fetch command.",

"description": "DEPRECATED: THIS VALUE IS IGNORED. CONFIGURATION MOVED TO A LOCAL FILE USE ENVIRONMENT VARIABLE `CUSTOM_GIT_FETCH_CONF` TO SET FILE PATH. JSON array of configuration that maps from Git clone URL domain/path to custom git fetch command.",

Can't we still read this value if the envvar is not set for 2 releases just so we don't break the customers we know use this? Additionally I'd document that the envvar needs to be set on the gitserver container.

Please provide feedback on this new PR #42704

Co-authored-by: Ryan Slade <ryanslade@gmail.com>

evict · 2022-10-07T15:24:46Z

Closing this MR in favour of a simpler solution: #42704. This makes sure the customer can keep using it while others have the option of disabling it.

cla-bot bot added the cla-signed label Jul 28, 2022

evict force-pushed the dax/fix_229 branch from 951aa21 to a17f10a Compare August 1, 2022 11:54

daxmc99 and others added 5 commits August 4, 2022 14:10

Require env var for custom git fetch

f9cfe26

go import

e140799

Add PR to changelog

24bd19a

Add mock for test

67d928b

fetch customGitFetch config from file

6ab4df7

evict force-pushed the dax/fix_229 branch from a17f10a to 657d223 Compare August 4, 2022 12:11

evict requested review from unknwon, a team and DaedalusG August 4, 2022 12:11

Add deprecation notice, changelog item

7fcf7ea

evict force-pushed the dax/fix_229 branch from 657d223 to 7fcf7ea Compare August 4, 2022 12:26

fix up CHANGELOG

68831f4

andreeleuterio approved these changes Aug 4, 2022

View reviewed changes

evict marked this pull request as ready for review August 4, 2022 14:24

daxmc99 commented Aug 4, 2022

View reviewed changes

cmd/gitserver/server/customfetch_test.go Outdated Show resolved Hide resolved

move up t.Cleanup

229b9c2

unknwon approved these changes Aug 8, 2022

View reviewed changes

cmd/gitserver/server/customfetch.go Outdated Show resolved Hide resolved

cmd/gitserver/server/customfetch.go Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

cmd/gitserver/server/customfetch_test.go Outdated Show resolved Hide resolved

evict and others added 4 commits August 8, 2022 10:16

remove newline

b2420d8

Co-authored-by: Joe Chen <joe@sourcegraph.com>

newline

d2f9091

Co-authored-by: Joe Chen <joe@sourcegraph.com>

formatting

1eb06af

Co-authored-by: Joe Chen <joe@sourcegraph.com>

update changelog

fc04725

DaedalusG reviewed Aug 15, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

DaedalusG reviewed Aug 15, 2022

View reviewed changes

DaedalusG added 2 commits August 17, 2022 00:52

Update CHANGELOG.md

6859886

Update customGitFetch to reflect env var

4aa1de5

DaedalusG mentioned this pull request Aug 17, 2022

Update customGitFetch to reflect env var #40459

Closed

DaedalusG and others added 7 commits August 17, 2022 01:14

Update doc/admin/monorepo.md

6990f63

fix code block

0f1ddce

Update doc/admin/monorepo.md

1e1cea5

Co-authored-by: Joe Chen <joe@sourcegraph.com>

Merge branch 'main' into DoccustomGitFetch

776f3bb

make usage doc for customGitFetch

ddcfdbb

Merge branch 'main' into dax/fix_229

1dab9c4

Merge branch 'DoccustomGitFetch' into dax/fix_229

1015a9a

unknwon approved these changes Sep 1, 2022

View reviewed changes

DaedalusG approved these changes Sep 1, 2022

View reviewed changes

DaedalusG requested changes Sep 1, 2022

View reviewed changes

keegancsmith reviewed Sep 2, 2022

View reviewed changes

cmd/gitserver/server/customfetch_test.go Outdated Show resolved Hide resolved

schema/site.schema.json Outdated Show resolved Hide resolved

Update cmd/gitserver/server/customfetch_test.go

53e8712

Co-authored-by: Keegan Carruthers-Smith <keegan.csmith@gmail.com>

evict added 2 commits September 26, 2022 16:24

update deprecation notice

a844afb

fix pipelines

2ff2ed3

ryanslade reviewed Sep 26, 2022

View reviewed changes

Update schema/site.schema.json

b07359b

Co-authored-by: Ryan Slade <ryanslade@gmail.com>

evict closed this Oct 7, 2022

evict deleted the dax/fix_229 branch October 7, 2022 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require env var for custom git fetch #39630

Require env var for custom git fetch #39630

daxmc99 commented Jul 28, 2022 •

edited by evict

evict commented Aug 4, 2022 •

edited

sourcegraph-bot commented Aug 4, 2022 •

edited

unknwon left a comment

DaedalusG left a comment

unknwon commented Aug 17, 2022 •

edited

DaedalusG commented Sep 1, 2022 •

edited

daxmc99 commented Sep 1, 2022

DaedalusG left a comment

keegancsmith left a comment

andreeleuterio commented Sep 3, 2022

ryanslade Sep 26, 2022

keegancsmith Sep 27, 2022

daxmc99 Oct 10, 2022

evict commented Oct 7, 2022

	"description": "DEPRECATED: THIS VALUE WILL IS IGNORED. CONFIGURATION MOVED TO A LOCAL FILE USE ENVIRONMENT VARIABLE `CUSTOM_GIT_FETCH_CONF` TO SET FILE PATH. JSON array of configuration that maps from Git clone URL domain/path to custom git fetch command.",
	"description": "DEPRECATED: THIS VALUE IS IGNORED. CONFIGURATION MOVED TO A LOCAL FILE USE ENVIRONMENT VARIABLE `CUSTOM_GIT_FETCH_CONF` TO SET FILE PATH. JSON array of configuration that maps from Git clone URL domain/path to custom git fetch command.",

Require env var for custom git fetch #39630

Require env var for custom git fetch #39630

Conversation

daxmc99 commented Jul 28, 2022 • edited by evict

Test plan

evict commented Aug 4, 2022 • edited

sourcegraph-bot commented Aug 4, 2022 • edited

unknwon left a comment

Choose a reason for hiding this comment

DaedalusG left a comment

Choose a reason for hiding this comment

unknwon commented Aug 17, 2022 • edited

DaedalusG commented Sep 1, 2022 • edited

daxmc99 commented Sep 1, 2022

DaedalusG left a comment

Choose a reason for hiding this comment

keegancsmith left a comment

Choose a reason for hiding this comment

andreeleuterio commented Sep 3, 2022

ryanslade Sep 26, 2022

Choose a reason for hiding this comment

keegancsmith Sep 27, 2022

Choose a reason for hiding this comment

daxmc99 Oct 10, 2022

Choose a reason for hiding this comment

evict commented Oct 7, 2022

daxmc99 commented Jul 28, 2022 •

edited by evict

evict commented Aug 4, 2022 •

edited

sourcegraph-bot commented Aug 4, 2022 •

edited

unknwon commented Aug 17, 2022 •

edited

DaedalusG commented Sep 1, 2022 •

edited