feat: add --experiments flag #5767

johnstcn · 2023-01-18T11:48:51Z

What this does

This PR makes the following changes:

Deprecates the --experimental flag
Adds a new flag --experiments which supports passing multiple comma-separated values or a wildcard value.
Exposes a new endpoint /api/v2/experiments that returns the list of enabled experiments.
Deprecates the field Features.Experimental in favour of this new API.

Why this does

Currently we only support setting a global boolean --experimental. For safe long-term feature development, we need to have a way to allow developers or users to conditionally enable or disable individual experimental code paths. This enables this use-case.

How this does

The cleanest way I found to implement the wildcard expansion behaviour is to use a type alias and implement the wildcard expansion in the DeploymentConfig reflection logic. I'm open to other possibilities. EDIT: This is now done on startup in coderd.go.
The existing behaviour of setting --experimental=true is maintained for backward compatibility.
Specifying the experiment wildcard will expand it to all experiments listed in codersdk.ExperimentsAll. Developers may wish to define additional experiments but not include them in this set, and this is OK.
Elected to use a simple []string for this. I considered a map[string]bool but the performance gain for the current low value of N (<10) does not, to me, outweigh the complexity of the additional plumbing for Viper et al.

Other things this does

Added a fix from @mtojek for in the apidocgen relating to type aliases.
Modified the apitypings script to support generating slice types. The existing code for maps appears to work fine here.
Updated develop.sh to pass additional args after -- to $CODERD_SHIM.

How to test what this does

Run ./scripts/develop.sh -- --experiments='*' and observe the local VSCode button on a running workspace.
Hit /api/v2/experiments and note the return value ['vscode_local'].

You can specify nonexistent experiments and they will be accepted but have no effect. This will be important behaviour as we remove or promote experiments.

(Note: currently, the only experimental feature that exists is vscode_local. Others will come along in future.)

…val1...

codersdk/experiments.go

coderd/experiments_test.go

mtojek · 2023-01-18T12:03:07Z

scripts/apidocgen/markdown-template/responses.def

@@ -13,6 +13,9 @@
 		if (!ref) {
 			ref = content.schema.items["x-widdershins-oldRef"];
 		}
+		if (!ref) {
+			return content.schema.items.type;


coderd/coderdtest/swaggerparser.go

cli/deployment/config.go

codersdk/experiments.go

codersdk/features.go

cli/deployment/config.go

coderd/experiments_test.go

deansheather · 2023-01-18T14:24:42Z

codersdk/features.go

+	// DEPRECATED: use Experiments instead.
+	Experimental bool `json:"experimental"`


Would be nice to return experiments here as well to avoid the extra frontend request on each page load and the extra API endpoint

We can, but I want to be careful about conflating Features (which are, as far as I'm aware, enterprise-only) with Experiments.

site/src/api/types.ts

BrunoQuaresma

FE looks good to me 👍

mafredri

LGTM!

mafredri · 2023-01-18T16:27:48Z

docs/api/schemas.md

+| Name               | Type                                 | Required | Restrictions | Description                           |
+| ------------------ | ------------------------------------ | -------- | ------------ | ------------------------------------- |
+| `errors`           | array of string                      | false    |              |                                       |
+| `experimental`     | boolean                              | false    |              | Experimental use Experiments instead. |


This message is weird, but it'll be removed soon™️ so not sure it matters.

codersdk/deploymentconfig.go

mafredri · 2023-01-18T16:29:50Z

scripts/apitypings/main.go

@@ -288,7 +288,7 @@ func (g *Generator) generateOne(m *Maps, obj types.Object) error {
 			// type <Name> string
 			// These are enums. Store to expand later.
 			m.Enums[obj.Name()] = obj
-		case *types.Map:
+		case *types.Map, *types.Array, *types.Slice:


Is this still needed?

Makes sense since typescriptType supports slices.

coderd/experiments.go

… DeploymentConfig reflection

codersdk/experiments.go

Emyrk · 2023-01-18T17:06:16Z

scripts/apitypings/main.go

@@ -288,7 +288,7 @@ func (g *Generator) generateOne(m *Maps, obj types.Object) error {
 			// type <Name> string
 			// These are enums. Store to expand later.
 			m.Enums[obj.Name()] = obj
-		case *types.Map:
+		case *types.Map, *types.Array, *types.Slice:


Makes sense since typescriptType supports slices.

site/src/api/typesGenerated.ts

deansheather · 2023-01-18T17:18:33Z

cli/deployment/config.go

 		Experimental: &codersdk.DeploymentConfigField[bool]{
-			Name:  "Experimental",
-			Usage: "Enable experimental features. Experimental features are not ready for production.",
-			Flag:  "experimental",
+			Name:    "Experimental",
+			Usage:   "Enable experimental features. Experimental features are not ready for production.",
+			Flag:    "experimental",
+			Default: false,
+			Hidden:  true,
+		},


Should print a deprecation notice at the top of server.go. Or we could implement it as a "Deprecated string" on DeploymentConfigField

It's hidden. So it should not matter right?

Well to notify old deployments that still have the variable set that they need to unset it or it won't work soon

I mean printing a notice if the flag or env var is present

warning gets printed in initExperiments at the bottom of coderd.go; folks may or may not be looking for it though

bpmct

LGTM. Wanted to share some context around why we made an experimental flag and how the product team uses it.

The current workflow is: I have a weekly recurring calendar event to "review experimental features" so that we can ensure that features don't stay in an "experimental" state for over 2 weeks. I manually test the feature, and sometimes we'll share with community members too, but we always rely on internal QA (not customers) to deem the feature "stable." If a feature is in experimental for over 2 weeks, it probably deserves a retro since we don't want to keep anything in "alpha/untested" for too long.

We've been using the --experimental flag for a couple of months now and only had 1-2 features at a given time under the flag. However, we have some big features coming up so I totally see the benefits of enabling/disabling specific flags, both for us and our customers, so they can test a specific feature without enabling any other (unrelated and potentially risky) features.

I'm sure it has developer-experience benefits too. It will actually come in handy for me, to see at a glance what features are still marked as experimental via the new endpoint.

I'd like to propose we keep the same workflow so that features don't get "stuck" in experimental for too long, without an end in sight.

johnstcn · 2023-01-18T18:58:48Z

I'd like to propose we keep the same workflow so that features don't get "stuck" in experimental for too long, without an end in sight.

100% agree!

johnstcn added 2 commits January 18, 2023 11:17

feat: support multiple values for --experimental

76c7b91

develop.sh: allow passing additional args to backend API via -- arg1=…

626d81b

…val1...

johnstcn self-assigned this Jan 18, 2023

johnstcn requested review from a team and mafredri and removed request for a team January 18, 2023 11:49

johnstcn marked this pull request as ready for review January 18, 2023 11:49

johnstcn requested a review from a team as a code owner January 18, 2023 11:49

johnstcn requested review from BrunoQuaresma and removed request for a team January 18, 2023 11:49

johnstcn added 2 commits January 18, 2023 11:58

fix swaggerparser test

d93a373

make fmt

e82c306

mtojek reviewed Jan 18, 2023

View reviewed changes

mafredri reviewed Jan 18, 2023

View reviewed changes

cli/deployment/config.go Show resolved Hide resolved

codersdk/experiments.go Outdated Show resolved Hide resolved

codersdk/experiments.go Outdated Show resolved Hide resolved

codersdk/features.go Outdated Show resolved Hide resolved

johnstcn added 5 commits January 18, 2023 13:34

require AuthN on /api/v2/experiments

047771c

fixup! require AuthN on /api/v2/experiments

3ba27ea

fix nits

3ebb778

add type alias for codersdk.Experiment

2aeb93e

make it clear that Entitlements.Experimental is deprecated

53d92d7

deansheather reviewed Jan 18, 2023

View reviewed changes

BrunoQuaresma reviewed Jan 18, 2023

View reviewed changes

site/src/api/types.ts Outdated Show resolved Hide resolved

BrunoQuaresma approved these changes Jan 18, 2023

View reviewed changes

johnstcn added 2 commits January 18, 2023 15:34

remove unnecessary types as they are now autogenerated

ef0cbc3

apply suggestions from code review

8fdb21b

johnstcn changed the title ~~feat: support multiple values for --experimental~~ feat: add --experiments flag Jan 18, 2023

fixup! apply suggestions from code review

0cb9a6a

mtojek self-requested a review January 18, 2023 16:28

mtojek approved these changes Jan 18, 2023

View reviewed changes

mafredri approved these changes Jan 18, 2023

View reviewed changes

warn about dragons

44e9638

johnstcn added 2 commits January 18, 2023 16:37

fix apidoc type

6c32aa5

codersdk.Experiments no longer needs be flaggable or special-cased in…

1cbaaf7

… DeploymentConfig reflection

Emyrk reviewed Jan 18, 2023

View reviewed changes

site/src/api/typesGenerated.ts Show resolved Hide resolved

lower-case when ingesting and comparing experiments

67d07f3

Emyrk approved these changes Jan 18, 2023

View reviewed changes

update apitypings unit tests

196b263

deansheather approved these changes Jan 18, 2023

View reviewed changes

bpmct approved these changes Jan 18, 2023

View reviewed changes

johnstcn merged commit 56b9965 into main Jan 18, 2023

johnstcn deleted the cj/config-experimental-multiple-values branch January 18, 2023 19:12

github-actions bot locked and limited conversation to collaborators Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add --experiments flag #5767

feat: add --experiments flag #5767

johnstcn commented Jan 18, 2023 •

edited

Loading

mtojek Jan 18, 2023

deansheather Jan 18, 2023

johnstcn Jan 18, 2023

BrunoQuaresma left a comment

mafredri left a comment

mafredri Jan 18, 2023

mafredri Jan 18, 2023

johnstcn Jan 18, 2023

Emyrk Jan 18, 2023

Emyrk Jan 18, 2023

deansheather Jan 18, 2023

Emyrk Jan 18, 2023

deansheather Jan 18, 2023

deansheather Jan 18, 2023

johnstcn Jan 18, 2023

bpmct left a comment •

edited

Loading

johnstcn commented Jan 18, 2023

		// DEPRECATED: use Experiments instead.
		Experimental bool `json:"experimental"`

feat: add --experiments flag #5767

feat: add --experiments flag #5767

Conversation

johnstcn commented Jan 18, 2023 • edited Loading

What this does

Why this does

How this does

Other things this does

How to test what this does

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoQuaresma left a comment

Choose a reason for hiding this comment

mafredri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpmct left a comment • edited Loading

Choose a reason for hiding this comment

johnstcn commented Jan 18, 2023

johnstcn commented Jan 18, 2023 •

edited

Loading

bpmct left a comment •

edited

Loading