[config]: Removal of Global externalURL and Updated Config Validation by cesrjimenez · Pull Request #57064 · sourcegraph/sourcegraph-public-snapshot

cesrjimenez · 2023-09-26T20:44:35Z

This Pull Request focuses on removing the reliance on global variables and enhancing configuration validation by utilizing the ContributeValidator pattern, leading to a more robust and maintainable system.

Changes:

Removal of Global externalURL References:
- Eradicated all references to the global externalURL. Now, the system efficiently retrieves the most recent externalURL using conf.Get(), ensuring that the most up-to-date and relevant URL is always utilized.
Updated Site Config Validation:
- Transitioned the validation of the site configuration to use the ContributeValidator pattern. This change replaces the prior global watch method, enhancing the reliability and structure of our configuration validation system.
Tests Update:
- All relevant tests have been updated to accommodate these significant changes. Ensured that all tests pass with the new implementations, maintaining our high standard of reliability and robustness.

Advantages:

Enhanced Efficiency: By using conf.Get() to retrieve externalURL, the system ensures the use of the most current URL, enhancing accuracy and responsiveness.
Improved Validation: Utilizing the ContributeValidator pattern for site config validation optimizes the process, making it more reliable and structured.
Maintainability: These changes significantly enhance the maintainability of the code, aligning with best practices and current codebase design patterns.

Test plan

Thorough testing conducted to ensure stability and functionality post-changes.
Adjusted existing tests to work with the new implementations, further ensuring system robustness.

sourcegraph-bot · 2023-09-27T17:15:11Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff b150ded...68fd6d7.

Notify	File(s)
@unknwon	cmd/frontend/internal/auth/oauth/BUILD.bazel cmd/frontend/internal/auth/oauth/provider.go cmd/frontend/internal/auth/oauth/provider_test.go cmd/frontend/internal/auth/openidconnect/BUILD.bazel cmd/frontend/internal/auth/openidconnect/middleware_test.go cmd/frontend/internal/auth/openidconnect/provider.go cmd/frontend/internal/auth/sourcegraphoperator/middleware_test.go

camdencheek · 2023-10-03T16:53:30Z

cmd/frontend/external/globals/globals.go

-func ExternalURL() *url.URL {
-	return globals.ExternalURL()
-}


lol, not sad to see this go away

camdencheek · 2023-10-03T17:12:00Z

cmd/repo-updater/shared/main.go

 		server.ChangesetSyncRegistry = syncRegistry
 	}

-	go globals.WatchExternalURL()


camdencheek

LGTM! Since you went through all the usage sites of ExternalURL, do you know if we can safely remove it from the "requires restart" list?

camdencheek · 2023-10-03T17:14:28Z

internal/conf/computed.go

+		var err error
+		if _, err = url.Parse(val); err != nil {
+			problems = append(problems, NewSiteProblem("Could not parse `externalURL`."))
+		}


nit: why the bonus err declaration? And should we include the error message

Suggested change

var err error

if _, err = url.Parse(val); err != nil {

problems = append(problems, NewSiteProblem("Could not parse `externalURL`."))

}

if _, err := url.Parse(val); err != nil {

problems = append(problems, NewSiteProblem(fmt.Sprintf("Could not parse `externalURL`: %s", err)))

}

camdencheek · 2023-10-03T17:16:42Z

internal/conf/computed.go

 		log.Fatalf("The 'DEPLOY_TYPE' environment variable is invalid. Expected one of: %q, %q, %q, %q, %q, %q, %q. Got: %q", deploy.Kubernetes, deploy.DockerCompose, deploy.PureDocker, deploy.SingleDocker, deploy.Dev, deploy.Helm, deploy.App, deployType)
 	}

+	ContributeValidator(validateExternalURLConfig)


We should check when this validator actually runs. If we can guarantee that conf.Get() will never see a config that doesn't pass validation, it would be nice to add a little helper to this package that parses the URL and panics on error. We added quite a bit of error handling in this PR for something that shouldn't ever happen.

When looking into this, I came across these, which can be rolled into the external URL validator.

Okay, I looked into this, and we do not, in fact, guarantee that the output of conf.Get() passes validation. We guarantee that the input passes validation, but we might add new validators with a release, which is usually the source of the banner message.

Feel free to ignore the first message. It think it's still worth moving those linked lines to the contributed validator though.

Sounds good, I moved the linked lines to the contributed validator 👍

camdencheek · 2023-10-03T17:17:31Z

internal/scim/user_service.go

 		// and make the user SCIM-controlled (which is the same as a replace)
 		return u.Update(ctx, strconv.Itoa(int(userID)), func(getResource func() scim.Resource) (updated scim.Resource, _ error) {
-			var now = time.Now()
+			now := time.Now()


keegancsmith

Have you tested this on a fresh setup with dev config/etc? conf.Get() on a fresh instance I think just returns an empty struct, which means externalURL will be empty. From reading the old behaviour, this is a divergence and I worry this will lead to us bricking sourcegraph on fresh startup or if an admin fat fingers. The old behaviour:

If there is bad or no config, we return http://example.com
If in the process life time there was a good value before the bad value, it returns the last good value.

The 2nd point doesn't seem that important, but the first seems like it may be very important.

Either way this is a heroic change. Approving given my concerns may not count for much.

remove global external url

ee70008

cla-bot bot added the cla-signed label Sep 26, 2023

bazel configure

364b1c6

cesrjimenez changed the title ~~Removal of Global externalURL and Updated Config Validation~~ [config]: Removal of Global externalURL and Updated Config Validation Sep 26, 2023

cesrjimenez added 4 commits September 26, 2023 15:16

removing additonal references

432e7f5

updating tests

f860d01

bazel tests

054a166

fixing tests

d2caf97

cesrjimenez marked this pull request as ready for review September 27, 2023 17:13

cesrjimenez requested review from a team and keegancsmith October 3, 2023 15:32

camdencheek reviewed Oct 3, 2023

View reviewed changes

cmd/repo-updater/shared/main.go

server.ChangesetSyncRegistry = syncRegistry

}

go globals.WatchExternalURL()

Copy link
Copy Markdown

Member

camdencheek Oct 3, 2023

Choose a reason for hiding this comment

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

🔥

camdencheek approved these changes Oct 3, 2023

View reviewed changes

updating validator for additional checks

68fd6d7

keegancsmith approved these changes Oct 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[config]: Removal of Global externalURL and Updated Config Validation#57064

[config]: Removal of Global externalURL and Updated Config Validation#57064
cesrjimenez wants to merge 7 commits intomainfrom
cesar/refactor-global-external-url

cesrjimenez commented Sep 26, 2023

Uh oh!

sourcegraph-bot commented Sep 27, 2023 •

edited

Loading

Uh oh!

camdencheek Oct 3, 2023

Uh oh!

camdencheek Oct 3, 2023

Uh oh!

camdencheek left a comment

Uh oh!

camdencheek Oct 3, 2023

Uh oh!

camdencheek Oct 3, 2023

Uh oh!

camdencheek Oct 3, 2023

Uh oh!

camdencheek Oct 3, 2023 •

edited

Loading

Uh oh!

cesrjimenez Oct 3, 2023

Uh oh!

camdencheek Oct 3, 2023

Uh oh!

keegancsmith left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

cesrjimenez commented Sep 26, 2023

Changes:

Advantages:

Test plan

Uh oh!

sourcegraph-bot commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camdencheek Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

camdencheek Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

camdencheek left a comment

Choose a reason for hiding this comment

Uh oh!

camdencheek Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

camdencheek Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

camdencheek Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

camdencheek Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cesrjimenez Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

camdencheek Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

keegancsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sourcegraph-bot commented Sep 27, 2023 •

edited

Loading

camdencheek Oct 3, 2023 •

edited

Loading