oneclickexport: add core export functionality #39813
Conversation
sashaostrikov
force-pushed
the
ao/one-click-export-core
branch
from
cf328a5 to
b0cc089
Compare
This includes ExportRequest processing, site config fetching and redacting and creating a zip archive.
sashaostrikov
force-pushed
the
ao/one-click-export-core
branch
from
b0cc089 to
1577a94
Compare
|
|
||
| configBytes := []byte(siteConfig.Site) | ||
|
|
||
| err = ioutil.WriteFile(dir+"/site-config.txt", configBytes, 0644) |
There was a problem hiding this comment.
any reason why .txt instead of .json?
There was a problem hiding this comment.
not really, it is just how usually we get this files in customer issues. basically doesn't matter, IMO, I think valid JSONs can have .json, and other text files will be with .txt
There was a problem hiding this comment.
Left some comments. No blockers.
Sidenote, when introducing totally new code it would be nice to add a short write up on how the new components interact and work with each other. The overview will help reviewers seeing the code for the first time build a mental model.
| // Export function accepts an ExportRequest and returns bytes of a zip archive | ||
| // with requested data |
There was a problem hiding this comment.
Nit: Edited for brevity and full sentences.
| // Export function accepts an ExportRequest and returns bytes of a zip archive | |
| // with requested data | |
| // Export accepts an ExportRequest and returns bytes of a zip archive | |
| // with requested data. |
|
|
||
| type OneClickExporter struct { | ||
| logger log.Logger | ||
| cfgProcessors map[string]Processor[ConfigRequest] |
There was a problem hiding this comment.
Nit: Is this easier to read?
| cfgProcessors map[string]Processor[ConfigRequest] | |
| configProcessors map[string]Processor[ConfigRequest] |
| // 1) tmp directory is created (exported files will end up in this directory and this directory is zipped in the end) | ||
| // 2) ExportRequest is read and each corresponding processor is invoked | ||
| // 3) Tmp directory is zipped after all the Processors finished their job | ||
| func (e *OneClickExporter) Export(request *ExportRequest) ([]byte, error) { |
There was a problem hiding this comment.
Any specific reason to pass the arg as a pointer? Can we pass this as value instead? Do you anticipate this struct to be particularly large in the near future?
There was a problem hiding this comment.
in fact it should not be a big one. plenty of fields -- yes. but most of them are just strings under 30 characters, ints and bools, so I think we can remove the pointer here 👍
| // 3) Tmp directory is zipped after all the Processors finished their job | ||
| func (e *OneClickExporter) Export(request *ExportRequest) ([]byte, error) { | ||
| // 1) creating a tmp dir | ||
| dir, err := ioutil.TempDir(os.TempDir(), "export-*") |
There was a problem hiding this comment.
Let's use the timestamp as the suffix instead of export-*, that is (export-%s, time.Now()).
This should avoid conflicts when the directory already exists if there's a chance that multiple export jobs have been triggered simultaneously.
There was a problem hiding this comment.
There will not be any conflicts, that's why I picked this function. Here is its doc: https://sourcegraph.com/github.com/golang/go/-/blob/src/os/tempfile.go?L78-80
There was a problem hiding this comment.
Oh TIL! Thanks for the link. Can you add a note on why the -* in the pattern in that case for future code readers?
| return nil | ||
| } | ||
|
|
||
| // creating file header |
There was a problem hiding this comment.
Nit: Comments are usually better read in the the imperative mood (create, not creating) similar to commit messages.
| // 1) tmp directory is created (exported files will end up in this directory and this directory is zipped in the end) | ||
| // 2) ExportRequest is read and each corresponding processor is invoked | ||
| // 3) Tmp directory is zipped after all the Processors finished their job | ||
| func (e *OneClickExporter) Export(request *ExportRequest) ([]byte, error) { |
There was a problem hiding this comment.
I'm noticing a lot of redundancy in the naming here which could make it hard when imported and used by other packages. For example:
exporter := oneclickexport.OneClickExporter{}
exporter.Export()
How about something like this instead:
OneClickExporter -> Agent / Actor
Export -> Run
And then this might look like:
exportAgent := oneclickexport.Agent{}
exportAgent.Run()
Or:
exportActor := oneclickexport.Actor{}
exportActor.Run()
There was a problem hiding this comment.
I'd like to stick to Exporter interface and Export function, because it perfectly describes what is happening.
oneclickexport.OneClickExporter is indeed too much, but Actor or Agent is a bit too abstract IMO, I'll think for a better naming of this one
|
|
||
| // Process function of SiteConfigProcessor loads site config, redacts the secrets | ||
| // and stores it in a provided tmp directory dir | ||
| func (g SiteConfigProcessor) Process(_ *ConfigRequest, dir string) { |
There was a problem hiding this comment.
Same question as above in regards to pointer vs passing the value directly?
|
@indradhanush I am merging this PR to unblock other dependent PRs. Basically, I've addressed/replied to all of your commits. If there is anything more, I'll fix it in next commits :) |
This is the first PR of 1-click export effort.
This includes ExportRequest processing, site config fetching and redacting and creating a zip archive.
Closes https://github.com/sourcegraph/sourcegraph/issues/39616 and https://github.com/sourcegraph/sourcegraph/issues/39619
Test plan
New test is added.