Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

batches: Use Draft instead of WIP in GitLab >= 14.0#42024

Merged
BolajiOlajide merged 18 commits into
mainfrom
bo/use-draft-gitlab-15
Oct 4, 2022
Merged

batches: Use Draft instead of WIP in GitLab >= 14.0#42024
BolajiOlajide merged 18 commits into
mainfrom
bo/use-draft-gitlab-15

Conversation

@BolajiOlajide

@BolajiOlajide BolajiOlajide commented Sep 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #41796

Test plan

  • Added unit test to assert the functionality change.

@BolajiOlajide BolajiOlajide self-assigned this Sep 23, 2022
@cla-bot cla-bot Bot added the cla-signed label Sep 23, 2022
@BolajiOlajide BolajiOlajide marked this pull request as draft September 23, 2022 17:09
@sourcegraph-bot

sourcegraph-bot commented Sep 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff d5d563a...2c68dd9.

Notify File(s)
@eseliger enterprise/internal/batches/sources/gitlab.go
enterprise/internal/batches/sources/gitlab_test.go
enterprise/internal/batches/sources/testdata/golden/GitlabSource_LoadChangeset_found
internal/extsvc/gitlab/client.go
internal/extsvc/gitlab/merge_requests.go
internal/extsvc/gitlab/merge_requests_test.go
internal/extsvc/gitlab/mock.go
internal/extsvc/gitlab/version.go
internal/extsvc/gitlab/webhooks/merge_requests.go
internal/extsvc/versions/mock.go
internal/extsvc/versions/store.go
internal/extsvc/versions/sync.go
internal/extsvc/versions/sync_test.go

@BolajiOlajide BolajiOlajide marked this pull request as ready for review September 24, 2022 15:33
@BolajiOlajide BolajiOlajide requested a review from a team September 24, 2022 15:49
eseliger
eseliger reviewed Sep 26, 2022
View reviewed changes
Comment thread enterprise/internal/batches/sources/gitlab.go
Piszmog
Piszmog approved these changes Sep 26, 2022
View reviewed changes
Comment thread enterprise/internal/batches/sources/gitlab.go Outdated
LawnGnome
LawnGnome reviewed Sep 26, 2022
View reviewed changes

@LawnGnome LawnGnome left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's here LGTM, but I am concerned about the way we're determining the GitLab version in GitLabSource, as described below in an inline comment.

I saw that in an earlier version of this PR we actually invoked GetVersion on the code host — was there a reason we dropped that? (sorry, I somehow missed Erik's post above)

One alternative idea here is that we do actually know what external service(s) are being queried when the version map is being built, so we could extend Version to also include some sort of identifier we can use to get a specific code host's version without calling GetVersion, and then have a fallback to do that on sites that are mid-upgrade and don't have the new field(s) yet in Redis.

Thoughts?

Comment thread enterprise/internal/batches/sources/gitlab.go
Comment thread enterprise/internal/batches/sources/gitlab.go Outdated
Comment thread internal/extsvc/gitlab/merge_requests.go
@eseliger

eseliger commented Sep 27, 2022

Copy link
Copy Markdown
Member

One alternative idea here is that we do actually know what external service(s) are being queried when the version map is being built, so we could extend Version to also include some sort of identifier we can use to get a specific code host's version without calling GetVersion, and then have a fallback to do that on sites that are mid-upgrade and don't have the new field(s) yet in Redis.

I like that!

@BolajiOlajide BolajiOlajide force-pushed the bo/use-draft-gitlab-15 branch from 90a75e9 to be0c7f4 Compare October 1, 2022 16:40
LawnGnome
LawnGnome approved these changes Oct 3, 2022
View reviewed changes

@LawnGnome LawnGnome left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks for taking the time to make the version detection work on a per-external service basis. LGTM!

eseliger
eseliger approved these changes Oct 3, 2022
View reviewed changes

@eseliger eseliger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, a few last questions.

Comment thread internal/extsvc/gitlab/merge_requests.go Outdated
Comment thread internal/extsvc/gitlab/merge_requests.go Outdated
Comment thread internal/extsvc/gitlab/merge_requests.go
Comment thread internal/extsvc/gitlab/merge_requests.go
Comment thread enterprise/internal/batches/sources/gitlab.go Outdated
@BolajiOlajide BolajiOlajide merged commit 6f5da28 into main Oct 4, 2022
@BolajiOlajide BolajiOlajide deleted the bo/use-draft-gitlab-15 branch October 4, 2022 14:25
@BolajiOlajide BolajiOlajide mentioned this pull request Oct 4, 2022
Merged
@BolajiOlajide BolajiOlajide mentioned this pull request Oct 11, 2022
Merged
BolajiOlajide added a commit that referenced this pull request Oct 11, 2022
@BolajiOlajide
remove key field from codehost json field (#42838)
bfc329b 
[Context](https://sourcegraph.slack.com/archives/C07KZF47K/p1665431134849859)

#42024 added an extra field to the [Version struct for external services](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@main/-/blob/internal/extsvc/versions/store.go?L16%3A1-20%3A2=&utm_source=VSCode-2.2.12), this field is used merely as an identifier to recognize different GitLab versions. This stuct was used to send ping data for `code_host_versions` and the `Key` field was sent as part of the ping payload (which shouldn't be).

This PR ensures the field isn't being sent when we marshal the versions for sending pings.

## Test plan

<!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles -->
Check the `code_host_versions` ping data and confirm the field isn't being sent.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@eseliger eseliger eseliger approved these changes

@Piszmog Piszmog Piszmog approved these changes

@LawnGnome LawnGnome LawnGnome approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@BolajiOlajide BolajiOlajide

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use Draft instead of WIP in GitLab >= 14.0

5 participants

@BolajiOlajide @sourcegraph-bot @eseliger @LawnGnome @Piszmog