handle scenario of multiple documents in single yaml file

[andxu]

No description provided.

[coveralls]

Coverage increased (+0.6%) to 69.852% when pulling 8d18268 on andxu:andy_multi_schema into 9b80bcc on redhat-developer:master.

[itowlson]

@JPinkney This looks like it might have regressed, maybe during the split into jsonSchema04 and jsonSchema07 and the switchover to use the built-in JSON language service. The Kubernetes extension has customers reporting that they no longer get intellisense on multi-document YAML files, and it seems like we are returning a schemaSequence but are no longer receiving callbacks for the sequence members.

vscode-kubernetes-tools/vscode-kubernetes-tools#633

[itowlson]

I looked at the test suite for multiple documents and it is rather ominously commented out... (Not that my own tests are in any state to throw shade! Just reinforcing my suspicion that this might have fallen between the cracks during the refactoring.)

[itowlson]

@JPinkney bump I had a bit of a look at fixing the regression but couldn't really figure out where to start. I appreciate you are strapped for resource but if you don't have time to look at it, could you give me some pointers so I can try to progress it please? Thanks!

cc @squillace @andxu

[JPinkney]

@itowlson It's weird I've just downloaded a fresh version of vscode-yaml and yaml-language-server and then ran vscode-yaml, installed the kubernetes extension and created a yaml file with:

apiVersion: v1
kind: Pod
metadata:
  name: myapp
  labels:
    name: myapp
spec:
  containers:
  - name: myapp
    image: <Image>
    resources:
      limits:
        memory: "128Mi"
        cpu: "500m"
    ports:
      - containerPort: 32323
...
---
apiVersion: v1
kind: Deployment
metadata:
  name: test

and intellisense etc are all working for me in this case

But if I change the kind in the second yaml to be kind: Pod instead, it doesn't work. For some reason the kubernetes extension sometimes sends back as schemaSequence and other times it doesn't (when in multi doc mode).

I think schemaSequence ended up getting removed because it could no longer be supported (previously we wrote hover, validation, etc from scratch so it was easier to extend but now we just depend on vscode-json-languageservice for a lot of the calls so it no longer had the ability to be integrated and it looks like it got removed as part of that whole refactoring)

I think instead of returning schemaSequence you might be able to return a schema that's something like:

{
    "oneOf": [
      { "$ref": "kubernetes://schema/v1@pod" },
      { "$ref": "kubernetes://schema/v1@deployment" }
    }
  ]
}

E.g. something like: https://github.com/Azure/vscode-kubernetes-tools/compare/master...JPinkney:fix-multi-doc-support?expand=1

If that doesn't work I'll try and think of some other things that might be able to help

[itowlson]

Thanks for investigating and for identifying the strange difference in behaviours. I’ll try tracing the sequence of calls with different documents including your two and see what they are doing.

I don’t know if oneOf will work for us because it loses the ‘and they are in this order’ aspect of schemaSequence but I’ll give it a go and see if it works in the 99% case. Thanks!

[itowlson]

Okay, your examples don't work for me, but thank heavens you posted the video, because I can see what the difference is.

In your video, you create a Pod manifest. This causes the k8s extension to tell the YAML extension to use the pod schema for that TextDocument, and when YAML asks us to send the pod schema content we oblige. Then when you continue editing the TextDocument by adding a Deployment, the pod schema remains in effect and is applied to the second YAMLDocument in the TextDocument. It looks like the amount that you write isn't sufficient to hit a difference between pods and deployments, so it looks like the deployment schema is working but I think it's just the pod schema also being applied to the deployment being created.

But what I've been doing is loading multi-documents from disk. Now when this happens, the k8s extension sees both YAMLDocuments, and tells the YAML extension to use a schemaSequence (whether pod+deployment or pod+pod). The YAML extension is befuddled by this and never calls back to request schema content.

So I think if you save, close and reload one of the manifests you created, you'll see the same problem I and k8s users do.

I'll try the oneOf strategy. @andxu's comments in the k8s source code imply he originally took this approach and I'm not sure why he was dissatisfied with it, but at this stage it may be better than nothing while we see if we can work out a longer term approach. We might need to find resource to bring back schemaSequence and see if we can make it work with the new JSON architecture. But I'll take a look.

Thanks as always for your time and your help!

[itowlson]

oneOf looks promising, but it looks like that was what we did before and it led to a bug (vscode-kubernetes-tools/vscode-kubernetes-tools#341, "the editor shows an error saying Matches multiple schemas when only one must validate. and not validation happens") that @andxu diagnosed as follows:

The problems are caused by the differences between json and yaml document: one json file only has one document while one yaml file can have multiple documents, in this case(multiple document), the current behavior for k8s schema provider is to provide anyOf [Deployment, ConfigMap] for the specified yaml, this works well if no validation problems/errors found, but if any errors are founded, for example the second ConfigMap has some validation errors, then the yaml-languange-server will not be able to detect which schema is actually chosen since both [Deployment, ConfigMap] schema is not acceptable, and it will use the first one Deployment to report the errors. The solution to this scenario is to notice the getSchemaForResource may return an Array if multiple documents are detected rather than the tricky anyOf schema to represent multiple documents, I am preparing for the PR. (emphasis added)

Here's the PR that made the change on the k8s side: vscode-kubernetes-tools/vscode-kubernetes-tools#353

So maybe oneOf worked on the happy path but not on the sad path... will test some more.

[itowlson]

cc @andxu

After switching to oneOf, I tried introducing some validation errors and didn't get the "matches multiple schemas" error and validation seemed to work correctly (and I got appropriate code completions). So maybe the problem with oneOf has gone away, perhaps due to the refactoring work on the RH YAML side. I wish I understood the original problem better though, so I could be sure!

[andxu]

@itowlson I will look at this issue

[andxu]

@JPinkney @itowlson, I have tested the latest version of vscode-yaml and vscode-kubernetes-tools with the code changes at https://github.com/Azure/vscode-kubernetes-tools/compare/master...JPinkney:fix-multi-doc-support?expand=1, unfortunately, the problem I pointed :

The problems are caused by the differences between json and yaml document: one json file only has one document while one yaml file can have multiple documents, in this case(multiple document), the current behavior for k8s schema provider is to provide anyOf [Deployment, ConfigMap] for the specified yaml, this works well if no validation problems/errors found, but if any errors are founded, for example the second ConfigMap has some validation errors, then the yaml-languange-server will not be able to detect which schema is actually chosen since both [Deployment, ConfigMap] schema is not acceptable, and it will use the first one Deployment to report the errors. The solution to this scenario is to notice the getSchemaForResource may return an Array if multiple documents are detected rather than the tricky anyOf schema to represent multiple documents

The problem can be shortly described as oneOf schema ignores the order of the schema, while the schemaSequence I added honors the orders, so for this schema:

{
    "oneOf": [
      { "$ref": "kubernetes://schema/v1@pod" },
      { "$ref": "kubernetes://schema/v1@deployment" }    
  ]
}

It will work normally if no errors are found, eg:

apiVersion: v1
kind: Pod
metadata:
  name: myapp
  labels:
    name: myapp
spec:
  containers:
  - name: myapp
    image: <Image>
    resources:
      limits:
        memory: "128Mi"
        cpu: "500m"
    ports:
      - containerPort: 32323
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: myapp
spec:
  selector:
    matchLabels:
      app: myapp
  template:
    metadata:
      labels:
        app: myapp
    spec:
      containers:
      - name: myapp
        image: <Image>
        resources:
          limits:
            memory: "128Mi"
            cpu: "500m"
        ports:
        - containerPort: 8989

While I change the last line to - containerPort: <Port>, the error message is wrong, and the description for property in second yaml is wrong, as it uses the first schema pod for validating the second yaml

apiVersion: v1
kind: Pod
metadata:
  name: myapp
  labels:
    name: myapp
spec:
  containers:
  - name: myapp
    image: <Image>
    resources:
      limits:
        memory: "128Mi"
        cpu: "500m"
    ports:
      - containerPort: 32323
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: myapp
spec:
  selector:
    matchLabels:
      app: myapp
  template:
    metadata:
      labels:
        app: myapp
    spec:
      containers:
      - name: myapp
        image: <Image>
        resources:
          limits:
            memory: "128Mi"
            cpu: "500m"
        ports:
        - containerPort: <Port>

[itowlson]

Thanks @andxu for the repro - I thought it must be something like this but was having a hard time finding the right way to produce an error!

@JPinkney how can we take this forward? I guess I should probably raise a new issue for it at minimum. grin

cc @squillace

[JPinkney]

@itowlson @andxu I'm currently looking into this but it looks like we might be able to get away with doing something like:
Augmenting currentDoc in this: https://github.com/redhat-developer/yaml-language-server/blob/master/src/languageservice/services/yamlHover.ts#L46 with some info about the document index (and any other info we might need) and then that calls jsonHover which then eventually calls https://github.com/redhat-developer/yaml-language-server/blob/master/src/languageservice/services/yamlSchemaService.ts#L30. From here I think we can do something like this when the schema is resolved:

if (schema.schema && schema.schema.schemaSequence && schema.schema.schemaSequence[currentDocIndex]) {
    return new SchemaService.ResolvedSchema(schema.schema.schemaSequence[currentDocIndex]);
}

WDYT?

That way when the schema is returned to the jsonHover, jsonValidation etc it's already working with the contents of the schemaSequence

We would have to modify validation and auto completion to behave the same way but I think it should work