chore: enhance dapryml schema validation #292
Conversation
| @@ -21,7 +21,8 @@ | |||
| }, | |||
| "appID": { | |||
| "description": "An ID for your application, used for service discovery.", | |||
| "type": "string" | |||
| "type": "string", | |||
| "pattern": "^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])\\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\\-]*[A-Za-z0-9])$" | |||
There was a problem hiding this comment.
I didn't found any official regex for validating app id. This regex was found on stackoverflow.
There was a problem hiding this comment.
For "standalone" (i.e. local) mode, it looks like daprd only disallows the use of periods (.) in app IDs (which is different than in Kubernetes mode, where the app ID must conform to the Kubernetes label format. I might use a pattern that only excludes periods.
| @@ -156,7 +157,10 @@ | |||
| }, | |||
| "env": { | |||
| "description": "Environment variables to be passed to the application.", | |||
| "type": "string" | |||
| "type": "object", | |||
There was a problem hiding this comment.
env is definitly not in string type and
it should be a map or object, so change the type to object.
| "type": "string" | ||
| "type": "object", | ||
| "additionalProperties": { | ||
| "type": ["string", "number", "boolean"] |
There was a problem hiding this comment.
Here I add "number" and "boolean" here for compatibility so that developers don't have to add quotes for cases like below:
env:
SOME_ENV1: true # compared with "true"
SOME_ENV2: 123 # compared with "123"
I'm not very sure about this change actually. We may discuss about it.
There was a problem hiding this comment.
I think that's fine, as long as those variations work as expected when the applications are run.
There was a problem hiding this comment.
It should be fine since dapr cli used yaml.unMarshal to deserialize the dapr.yaml file and it's compatible for boolean & int for string types.
https:/dapr/cli/blob/4e3ae81df53a87c4707e95f5c5dfd8295e230c80/pkg/standalone/runfileconfig/run_file_config_parser.go#L36
| @@ -165,7 +169,8 @@ | |||
| }, | |||
| "placementHostAddress": { | |||
| "description": "The host on which the placement service resides.", | |||
| "type": "string" | |||
| "type": "string", | |||
| "pattern": "^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])\\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\\-]*[A-Za-z0-9])$" | |||
There was a problem hiding this comment.
This one is similiar with the previous. I tried my best to find an regex here.
There was a problem hiding this comment.
The placement host can include an optional port value (e.g. can be localhost or localhost:5555). Using regular expressions for (even portions of) URLs can be tricky, especially if we have to consider both name and IP (v4/v6) address forms. We might avoid doing so for now.
package.json
Outdated
| @@ -12,6 +12,9 @@ | |||
| "workspace", | |||
| "ui" | |||
| ], | |||
| "extensionDependencies": [ | |||
| "redhat.vscode-yaml" | |||
There was a problem hiding this comment.
YAML file's intelli-sense depends on this extension. VS Code will notify developers about the installation of YAML extension during the activation stage of the Dapr extension.
There was a problem hiding this comment.
Because IntelliSense isn't a critical feature of the extension--helpful, sure, but not absolutely needed--I had intended not to have a hard dependency on the YAML extension. Unfortunately, there's no way for extensions themselves to just recommend extensions, which I believe would be a better experience. (VS Code itself has a mechanism for arbitrary recommendations, but it is something we would have to ask for and I don't know if there's a strong enough argument for it.)
If we wanted, we could hook the opening dapr.yaml files and then look to see whether the YAML extension is installed; if not, then we could display our own recommendation.
chore: add maximum limits for dapr.yaml port properties
| @@ -138,7 +145,7 @@ | |||
| "type": "integer", | |||
| "minimum": 0 | |||
| }, | |||
| "daprPath": { | |||
| "runtimePath": { | |||
There was a problem hiding this comment.
the daprPath has been changed to runtimePath in dapr/cli (dapr/cli#1203 (comment)), so here should change as well
|
While this is a good start, there's potentially a higher-level set of validation that could be useful to developers. For example, where a property requires a path, ensure that that path exists or, when the developer is adding the port, provide a default that matches the application settings. That's more semantic analysis rather than the syntactic done through this JSON schema and is done through language servers and related APIs. |
This PR comes to a stage when we can discuss specific topics.