-
Notifications
You must be signed in to change notification settings - Fork 73
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Feature Flag Schema 2.0.0 #841
Conversation
"$id": "#/properties/variants/items/properties/configuration_value", | ||
"type": ["string", "null", "number", "object", "array", "boolean"], | ||
"title": "Variant Configuration Value", | ||
"description": "The inline value set for this feature variant." |
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.
Do we need to specify the patternProperties for configuration_value
here (like parameters
of client_filters
)?
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.
From what I understand, parameters
always expects an object so it makes sense to have the patternProperties
, but I didn't know if I need it for this case if it's sometimes an object, but configuration_value
can be anything.
When I look online it seems to suggest it's not needed, but we have it explicitly say each allowed value for parameters
in our schema.
"description": "A container for metadata relevant to telemetry.", | ||
"patternProperties": { | ||
"^.*$": { | ||
"type": "string" |
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.
Do we prohibit properties being number or boolean here?
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.
In the feature definition TelemetryMetadata
is a Dictionary<string, string>
, so I thought it might be the same here.
Co-authored-by: Zhiyuan Liang <141655842+zhiyuanliang-ms@users.noreply.github.com>
"description": "A flag to enable or disable sending telemetry events.", | ||
"default": false | ||
}, | ||
"telemetry_metadata": { |
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.
This is not a desired property.
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.
I expect it is. We will insert this dynamically, but if someone hardcodes this in a feature flag, it will be honored.
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.
Why would a user not use tags on the key-value?
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.
Discussed with Zhenlan. If we were to have feature flags as a resource at the same level of key-values we would most likely have telemetry configuration on top of existing App Configuration paradigms such as tags and etag. Thus, it is reasonable to include it here not only for feature management client operability but also because it makes sense to exist on the server.
Although since we have multiple properties revolving around telemetry we discussed to switch the schema. Here is an example following the updated schema
{
"telemetry": {
"enabled": true,
"metadata": {
"foo": "bar"
}
}
}
"description": "A feature is OFF if enabled is false. If enabled is true, then the feature is ON if there are no conditions (null or empty) or if all conditions are satisfied.", | ||
"default": false | ||
}, | ||
"status": { |
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.
This is a .NET only concept. It doesn't exist in AppConfig schema (which uses the "enabled" property).
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.
Ok, our original variants documentation mentions adding it to the schema so I went off of that, but I can remove it. Does that mean the .NET provider is just going to use the enabled
property to send status
to featuremanagement?
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 "status" is a .NET FM only property designed to match the "enabled" property in the AppConfig schema. This is why it's included in the .NET FM document. In other words, the .NET FM document (implicitly) maintains another schema that is only used by the .NET FM library. But as you know, the .NET FM library is now adding support for the AppConfig schema too. The AppConfig schema is the one we plan to add support in FM libraries of all languages.
"title": "An Azure App Configuration Feature Declaration", | ||
"required": [ | ||
"id", | ||
"enabled", |
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.
If the enabled
property has a default value, it is not required. Do we require it in our provider/FM libraries?
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.
Portal doesn't let you omit enabled, but the provider will just set it to false by default
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.
If the library doesn't enforce it, it's not required. Looks like our new schema support PR doesn't require it either.
We should remove it from the required and all FM libraries should treat a feature flag as disabled if the property is not present.
"telemetry": { | ||
"$id": "#/properties/telemetry", | ||
"type": "object", | ||
"title": "Feature Telemetry Options", |
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.
We don't always prefix "Feature" for other top level properties. Feels redundant here.
Feature Telemetry Options
"$id": "#/properties/telemetry/properties/metadata", | ||
"type": "object", | ||
"title": "Telemetry Metadata", | ||
"description": "A container for metadata relevant to telemetry.", |
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.
"description": "A container for metadata relevant to telemetry.", | |
"description": "A container for metadata that should be bundled with flag telemetry.", |
"description": "The name of the variant to use if the calculated percentile for the current user falls in the provided range.", | ||
"pattern": "^(.*)$" | ||
}, | ||
"from": { |
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.
Should have min: 0, max: 100. Same with to.
"title": "Variants Collection", | ||
"description": "The list of variants defined for this feature. A variant represents a value of a feature flag that can be a string, a number, a boolean, or even a configuration object.", | ||
"title": "Variant Collection", | ||
"description": "The list of variants defined for this feature. A variant represents a configuration value of a feature flag that can be a string, a number, a boolean, or even a configuration object.", |
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.
NIT: no need to stress on configuration object "... or even a configuration object".
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.
A second thought, should it be "JSON object"? We don't support the configuration object to be anything else, do we?
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.
In terms of how it's defined in app config/local files, it should always be a JSON. I can change it to say that
No description provided.