This repository was archived by the owner on Sep 30, 2024. It is now read-only.
Make the URL dynamic, now with a fallback#62852
Merged
Merged
Conversation
chrsmith
approved these changes
May 22, 2024
Contributor
There was a problem hiding this comment.
This looks good to me, although with a small suggestion.
I agree with your thoughts on other ways to set the "defaults" for the codyProConfig data, and just updating the logic here in the code is probably the right call for now. (And after we ship the embedded UI, we can remove some of the logic here entirely, since it will just be a static route to some other page in the same React app.)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I made this URL dynamic before, here, but that didn't work because the "default" value in the
site.schema.jsonfile doesn't do what I thought it did. So @chrsmith did a quick fix in https://github.com/sourcegraph/sourcegraph/pull/62835, but then we lost the ability to make this dynamic.So this PR makes it dynamic again, this time with a working fallback.
Alternatives considered:
I tried to do the same thing on the back end, but it didn't feel right.
computed.go:But having the defaults there was meh, plus I had to introduce the new type
conftypes.CodyProConfigbut nothing prevents another eng to just use the config directly, which might become confusing.makeFrontendCodyProConfig, which might be better, but then, on the front end, we already have a single location where we access the SSC base URL, and we already had the default there, so I just went with that.Test plan
Here is a 1-min Loom where I test it with different settings and demo that it works as intended.