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

campaigns: Switch to changeset syncer per code host #11874

Merged
merged 9 commits into from Jul 3, 2020

Conversation

ryanslade
Copy link
Contributor

@ryanslade ryanslade commented Jul 1, 2020

We'll launch a syncer per code host Instead of per external service.

Syncer scheduling is coupled to rate limiting which is per code host so this change aligns them.

Instead of per external service. Syncer scheduling is coupled to rate
limiting which is per code host so this change aligns them.
@codecov
Copy link

codecov bot commented Jul 1, 2020

Codecov Report

Merging #11874 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #11874   +/-   ##
=======================================
  Coverage   50.44%   50.44%           
=======================================
  Files        1513     1513           
  Lines       89703    89703           
  Branches     6742     6742           
=======================================
  Hits        45255    45255           
  Misses      40477    40477           
  Partials     3971     3971           
Flag Coverage Δ
#go 54.85% <0.00%> (ø)
#storybook 10.85% <0.00%> (ø)
#typescript 36.76% <0.00%> (ø)
#unit 50.07% <0.00%> (ø)

@ryanslade ryanslade marked this pull request as ready for review July 2, 2020 08:59
@ryanslade ryanslade requested a review from a team as a code owner July 2, 2020 08:59
enterprise/internal/campaigns/syncer.go Outdated Show resolved Hide resolved
rawURL = v.Url
case *schema.GitHubConnection:
rawURL = v.Url
// Unsupported by campaigns
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this switch statement, can you make use of this map here to check whether the code host is supported?

// SupportedExternalServices are the external service types currently supported
// by the campaigns feature. Repos that are associated with external services
// whose type is not in this list will simply be filtered out from the search
// results.
var SupportedExternalServices = map[string]struct{}{
extsvc.TypeGitHub: {},
extsvc.TypeBitbucketServer: {},
}

That uses "Type*" constants, but since you have the external service at hand, maybe we can get to that from the "kind"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remembered that we had the BaseURL method on repos.ExternalService, but we're not using it anywhere.

So, I moved that to the extsvc package and renamed it to ExtractBaseURL so that we can use it with either repos.ExternalService or api.ExternalService. That allows us to remove the normalisedURLFromService function.

I also added a function so that we can check campaigns supports an external service "Kind" and use that in the syncer.Add method.

enterprise/internal/campaigns/syncer.go Outdated Show resolved Hide resolved
enterprise/internal/campaigns/syncer.go Outdated Show resolved Hide resolved
ryanslade and others added 5 commits July 2, 2020 13:48
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
Move it to the extsvc package so that we can also use it with
api.ExternalService types
@ryanslade ryanslade requested a review from a team July 3, 2020 10:20
@ryanslade
Copy link
Contributor Author

The DB backcompat test the only one failing but this change doesn't include any migrations so I'm going to merge it.

@ryanslade
Copy link
Contributor Author

ryanslade commented Jul 3, 2020

The DB backcompat test the only one failing but this change doesn't include any migrations so I'm going to merge it.

Ah, I don't have permissions. Looks like I'll probably need to do some merging from master first

@ryanslade ryanslade merged commit 51a804d into master Jul 3, 2020
@ryanslade ryanslade deleted the syncer-per-codehost branch July 3, 2020 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants