- Notifications
You must be signed in to change notification settings - Fork925
fix: do not warn on valid known experiments#18514
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
| exportconstisKnownExperiment=(experiment:string):boolean=>{ | ||
| returnExperiments.includes(experimentasExperiment); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Alternatively, we could have the FE return the list of known experiments, but it seems silly to do this when we can just use the auto-generated type.
| deploymentOptions:SerpentOption[]; | ||
| dailyActiveUsers:DAUsResponse|undefined; | ||
| readonlyinvalidExperiments:Experiments|string[]; | ||
| readonlysafeExperiments:Experiments|string[]; | ||
| readonlyinvalidExperiments:readonlystring[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: this has to be a string because it might not be an Experiment
| @@ -118,7 +118,7 @@ export const invalidExperimentsEnabled: Story = { | |||
| hidden:false, | |||
| }, | |||
| ], | |||
| safeExperiments:["shared-ports"], | |||
| safeExperiments:["example"], | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: using the "example" experiment enum here instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Makes sense to me. Using a consistent experiment that we don't need to keep changing.
I wonder ifexample is the best name, but I do not have a better suggestion.
ExperimentPlaceholder,ExperimentDummy, idk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The nomenclature aroundavailable andsafe feels inconsistent in the api and usage. But that is not introduced in this PR.
| @@ -972,7 +972,7 @@ func New(options *Options) *API { | |||
| }) | |||
| r.Route("/experiments",func(r chi.Router) { | |||
| r.Use(apiKeyMiddleware) | |||
| r.Get("/available",handleExperimentsSafe) | |||
| r.Get("/available",handleExperimentsAvailable) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Kinda unfortunate we have/available that returns only the safe experiments. Ideally there would be a/available and/safe. Or just have/available return amap[string]bool where thebool istrue for safe or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah I considered also returning all available experiments here but elected to keep the scope of this PR small.
| if!slice.Contains(codersdk.ExperimentsKnown,ex) { | ||
| log.Warn(context.Background(),"ignoring unknown experiment",slog.F("experiment",ex)) | ||
| }elseif!slice.Contains(codersdk.ExperimentsSafe,ex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍.
d892427 intomainUh oh!
There was an error while loading.Please reload this page.
Fixes#18024
handleExperimentsSafetohandleExperimentsAvailableto better match semanticscodersdk.ExperimentsKnownand updatesReadExperimentsto log on invalid experimentscodersdk.Experimentsso apitypings generates a valid enum list of possible values of experiment