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

[Backport 5.2] Updating an code host config maintains the correct unrestricted…#58795

Merged
keegancsmith merged 4 commits into
5.2from
backport-58772-to-5.2
Dec 7, 2023
Merged

[Backport 5.2] Updating an code host config maintains the correct unrestricted…#58795
keegancsmith merged 4 commits into
5.2from
backport-58772-to-5.2

Conversation

@pjlast

@pjlast pjlast commented Dec 6, 2023

Copy link
Copy Markdown
Contributor

Fixes an issue where the logic that determines whether or not a code host connection is unrestricted differs between the code host connection creation and update code.

Test plan

Tests updated

cc @sourcegraph/release-guild

@sourcegraph-bot

sourcegraph-bot commented Dec 6, 2023

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 30dea0d...4d7d122.

Notify File(s)
@eseliger internal/database/external_services.go
internal/database/external_services_test.go

@pjlast pjlast changed the title [fix] Updating an code host config maintains the correct unrestricted… [Backport 5.2] Updating an code host config maintains the correct unrestricted… Dec 6, 2023
@pjlast pjlast requested a review from a team December 6, 2023 08:53
@sourcegraph-bot

sourcegraph-bot commented Dec 6, 2023

Copy link
Copy Markdown
Contributor

📖 Storybook live preview

// `Unrestricted` and `HasWebhooks`.
func (e *externalServiceStore) recalculateFields(es *types.ExternalService, rawConfig string) error {
es.Unrestricted = !envvar.SourcegraphDotComMode() && !gjson.Get(rawConfig, "authorization").Exists()
func calcUnrestricted(config string) bool {

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.

I know this is a backport, so treat this as a driveby followup. But the mutliple checks on dotcom make this a little tricky to read. Seems like you could do something like "if dotcom return true" and then the rest of the logic may also be easier to read.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah gotcha, thanks! I'll make a follow-up!

@keegancsmith keegancsmith enabled auto-merge (squash) December 6, 2023 10:00
@keegancsmith

Copy link
Copy Markdown
Member

CI is not pleased.

@pjlast

pjlast commented Dec 6, 2023

Copy link
Copy Markdown
Contributor Author

@keegancsmith I see in the test I added I depended on a new field that wasn't backported. Updated the test, so 🤞

@keegancsmith keegancsmith merged commit 68cbcb9 into 5.2 Dec 7, 2023
@keegancsmith keegancsmith deleted the backport-58772-to-5.2 branch December 7, 2023 13:04
@varungandhi-src varungandhi-src mentioned this pull request Jan 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants