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

Rewrite precise-code-intel-bundle-manager in Go #9586

Merged
merged 15 commits into from Apr 23, 2020

Conversation

efritz
Copy link
Contributor

@efritz efritz commented Apr 6, 2020

This is a rewrite of the precise-code-intel-bundle-manager in Go. This does not change the current entrypoints as we're not ready to switch to the new code yet. I am planning on manually testing requests against the old and new servers to ensure the behavior is the same during usage. A follow-up PR(s) will remove the old code and switch to the new code in build and deployment environments.

This new code is fairly well tested (database: 79.2% coverage, janitor: 62.4% coverage, and types: 79.0%). The server code will be more fully tested before it replaces the existing TypeScript code.

This new code needs to emit metric, add tracing to requests, and wrap errors properly before being considered ready for production.

Opening PR now for discussion as it's a large chunk of code. This effort is tracked in #9964.

@efritz efritz changed the base branch from master to ef/move-lsifserver-client April 19, 2020 23:47
@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

Merging #9586 into master will increase coverage by 0.20%.
The diff coverage is 65.15%.

@@            Coverage Diff             @@
##           master    #9586      +/-   ##
==========================================
+ Coverage   42.74%   42.95%   +0.20%     
==========================================
  Files        1348     1354       +6     
  Lines       74083    74693     +610     
  Branches     6651     6651              
==========================================
+ Hits        31668    32081     +413     
- Misses      39562    39692     +130     
- Partials     2853     2920      +67     
Flag Coverage Δ
#unit 42.95% <65.15%> (+0.20%) ⬆️
Impacted Files Coverage Δ
enterprise/internal/campaigns/syncer.go 79.65% <ø> (ø)
internal/extsvc/bitbucketserver/events.go 0.00% <0.00%> (ø)
internal/httpcli/client.go 38.88% <ø> (ø)
internal/conf/conf.go 15.96% <40.00%> (-0.28%) ⬇️
...e-intel-bundle-manager/internal/janitor/janitor.go 51.02% <51.02%> (ø)
enterprise/internal/campaigns/webhooks.go 26.32% <52.27%> (+3.77%) ⬆️
...intel-bundle-manager/internal/database/database.go 61.23% <61.23%> (ø)
...de-intel-bundle-manager/internal/database/cache.go 72.72% <72.72%> (ø)
...e-intel-bundle-manager/internal/types/unmarshal.go 76.72% <76.72%> (ø)
cmd/gitserver/server/cleanup.go 67.94% <100.00%> (ø)
... and 11 more

@efritz efritz changed the base branch from ef/move-lsifserver-client to master April 20, 2020 13:34
@efritz efritz marked this pull request as ready for review April 20, 2020 14:11
@efritz efritz requested a review from slimsag as a code owner April 20, 2020 14:11
@efritz efritz requested review from a team April 20, 2020 14:11
@unknwon
Copy link
Member

unknwon commented Apr 20, 2020

I haven't done reviewing the PRs, just some notes (for discussions) on skimming the files:

  1. Is it too aggressive to log.Fatal for missing config values? Any chance we could just fall back to use a default? NVM, I see we've been using it in many places.
  2. We started using cmp.Diff instead of reflect.DeepEqual in new code, examples: https://sourcegraph.com/search?q=repo:%5Egithub%5C.com/sourcegraph/sourcegraph%24+cmp.Diff&patternType=literal
  3. The directory name for test data we're currently using is "testdata" (without dash).
  4. Would be nice to have a list of non-go files and how they will be used later, it isn't obvious to me so far.
  5. Nice examples with all the ^^^^^s 👍

Copy link
Member

@unknwon unknwon left a comment

Choose a reason for hiding this comment

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

Submitting my partial reviews so number of comments aren't overwhelming. Very enjoyable piece of code though!

@efritz efritz requested a review from unknwon April 21, 2020 14:11
Copy link
Member

@unknwon unknwon left a comment

Choose a reason for hiding this comment

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

Nice! Approve to unblock.

@efritz efritz requested a review from a team as a code owner April 22, 2020 15:04
Copy link
Member

@keegancsmith keegancsmith 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 didn't deeply review due to size. Rather just took a high level view over it to see anything that stood out. So sorry for only finding nits :) Nice work!

@efritz efritz merged commit 55a84d6 into master Apr 23, 2020
@efritz efritz deleted the port-bundle-manager-to-go branch April 23, 2020 20:02
notjrbauer pushed a commit to notjrbauer/sourcegraph that referenced this pull request Apr 25, 2020
@efritz efritz self-assigned this Apr 29, 2020
@efritz efritz added the team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) label Apr 29, 2020
@efritz efritz added this to the 3.16 milestone Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team/graph Graph Team (previously Code Intel/Language Tools/Language Platform)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants