Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: add --experiments flag#5767

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

Merged
johnstcn merged 17 commits intomainfromcj/config-experimental-multiple-values
Jan 18, 2023

Conversation

johnstcn
Copy link
Member

@johnstcnjohnstcn commentedJan 18, 2023•
edited
Loading

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 fieldFeatures.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 theDeploymentConfig reflection logic. I'm open to other possibilities. EDIT: This is now done on startup incoderd.go.
  • The existing behaviour of setting--experimental=true is maintained for backward compatibility.
  • Specifying the experiment wildcard will expand it to all experiments listed incodersdk.ExperimentsAll. Developers may wish to define additional experiments butnot include them in this set, and this is OK.
  • Elected to use a simple[]string for this. I considered amap[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 theapidocgen relating to type aliases.
  • Modified theapitypings script to support generating slice types. The existing code for maps appears to work fine here.
  • Updateddevelop.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 isvscode_local. Others will come along in future.)

ntimo and bpmct reacted with heart emoji
@johnstcnjohnstcn self-assigned thisJan 18, 2023
@johnstcnjohnstcn requested review froma team andmafredri and removed request fora teamJanuary 18, 2023 11:49
@johnstcnjohnstcn marked this pull request as ready for reviewJanuary 18, 2023 11:49
@johnstcnjohnstcn requested a review froma team as acode ownerJanuary 18, 2023 11:49
@johnstcnjohnstcn requested review fromBrunoQuaresma and removed request fora teamJanuary 18, 2023 11:49
@@ -13,6 +13,9 @@
if (!ref) {
ref = content.schema.items["x-widdershins-oldRef"];
}
if (!ref) {
return content.schema.items.type;
Copy link
Member

Choose a reason for hiding this comment

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

👍

Comment on lines +84 to +85
// DEPRECATED: use Experiments instead.
Experimental bool `json:"experimental"`
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma left a comment

Choose a reason for hiding this comment

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

FE looks good to me 👍

@johnstcnjohnstcn changed the titlefeat: support multiple values for --experimentalfeat: add --experiments flagJan 18, 2023
@mtojekmtojek self-requested a reviewJanuary 18, 2023 16:28
Copy link
Member

@mafredrimafredri left a comment

Choose a reason for hiding this comment

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

LGTM!

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

Choose a reason for hiding this comment

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

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

@@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yes.

mafredri reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense sincetypescriptType supports slices.

johnstcn reacted with thumbs up emoji
@@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense sincetypescriptType supports slices.

johnstcn reacted with thumbs up emoji
Comment on lines 450 to +456
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,
},
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

warning gets printed ininitExperiments at the bottom ofcoderd.go; folks may or may not be looking for it though

Copy link
Member

@bpmctbpmct left a comment•
edited
Loading

Choose a reason for hiding this comment

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

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
Copy link
MemberAuthor

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!

@johnstcnjohnstcn merged commit56b9965 intomainJan 18, 2023
@johnstcnjohnstcn deleted the cj/config-experimental-multiple-values branchJanuary 18, 2023 19:12
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJan 18, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@mafredrimafredrimafredri approved these changes

@EmyrkEmyrkEmyrk approved these changes

@deansheatherdeansheatherdeansheather approved these changes

@BrunoQuaresmaBrunoQuaresmaBrunoQuaresma approved these changes

@mtojekmtojekmtojek approved these changes

@bpmctbpmctbpmct approved these changes

Assignees

@johnstcnjohnstcn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@johnstcn@mafredri@BrunoQuaresma@Emyrk@deansheather@mtojek@bpmct
[8]ページ先頭
©2009-2025 Movatter.jp