Skip to content
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 schema info to diagnostics #326

Merged
merged 6 commits into from
Oct 15, 2020

Conversation

evidolob
Copy link
Collaborator

@evidolob evidolob commented Sep 30, 2020

Fix: #310

Also fix: redhat-developer/vscode-yaml#373 partially, as I add YAML for YAML syntax errors

Demo:
Screenshot 2020-09-30 at 14 46 57

Schema with title:
Screenshot 2020-09-30 at 14 47 30

Local schema:
Screenshot 2020-09-30 at 14 48 38

Yaml error:
Screenshot 2020-09-30 at 14 49 38

Problems:
Screenshot 2020-09-30 at 14 52 02

Kubernetos schema:
Screenshot 2020-10-13 at 12 20 06

@evidolob evidolob self-assigned this Sep 30, 2020
@coveralls
Copy link

coveralls commented Sep 30, 2020

Coverage Status

Coverage increased (+0.4%) to 78.016% when pulling 23529fd on evidolob:add-schema-to-diagnostics into b9fda68 on redhat-developer:master.

@JPinkney
Copy link
Contributor

JPinkney commented Sep 30, 2020

I think this is a really cool feature. I've been playing around with it and it seems like it works perfectly when theres one schema associated to the file but it has a bit of trouble when there's multiple schemas associated to a file.

E.g.

"yaml.schemas": {
    "kubernetes": "test.yaml",
    "https://json.schemastore.org/composer": "test.yaml"
}

and then with content

abandoned: v1
archive:
  exclude:
    asd: asd

you'll get yaml-schema: schemaservice://combinedschema/file%3A///home/joshpinkney/Documents/Work/Schemas/test.yaml in the error message when you'd expect the composer schema.

Also if you did:

"yaml.schemas": {
    "kubernetes": "prometheus.yaml"
},

with content

apiVersion: v1
kind: Pod
metadata:
  name: False

you'll get yaml-schema: This is because the prometheus.yaml schema from the schema store and the kubernetes schema are both applied to prometheus.yaml

@evidolob
Copy link
Collaborator Author

@JPinkney Thx, I miss that file may have multiple schemas, going to fix that

@evidolob evidolob marked this pull request as draft September 30, 2020 12:09
@evidolob evidolob marked this pull request as ready for review October 13, 2020 09:19
@evidolob
Copy link
Collaborator Author

@JPinkney
Now it should resolve schema correctly:
Screenshot 2020-10-13 at 12 20 56

Signed-off-by: Yevhen Vydolob <[email protected]>
@evidolob
Copy link
Collaborator Author

@gorkem @JPinkney @joshuawilson
Full k8s url:
Screenshot 2020-10-13 at 12 34 24

Copy link
Contributor

@JPinkney JPinkney left a comment

Choose a reason for hiding this comment

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

Generally, it looks good to me. I tried it out locally and everything seems to be working almost perfectly. I only issue I ran into was in a test scenario with a .drone.yml file where I had yaml.settings as:

"yaml.schemas": {
    "kubernetes": ".drone.yml"
},

with the content:

apiVersion: v1
kind: Deployment

where I was getting error message Property apiVersion is not allowed.YAML when hovering apiVersion and when hovering deployment I got

Value is not accepted. Valid values: "pipeline".: YAML

maybe in this case the validator matches a little bit of the drone ci and the kubernetes schema and cannot decide which one it is matching? I'm not sure

src/languageservice/services/yamlValidation.ts Outdated Show resolved Hide resolved
test/schemaValidation.test.ts Outdated Show resolved Hide resolved
if (uriString) {
const url = URI.parse(uriString);
if (url.scheme === 'file') {
label = url.fsPath;
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case won't label always just end up being whatever url.toString() is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is done for local files, I assume the path /home/user/folder/schema.json will be more informative than file:///home/user/folder/schema.json. If URI has different schema than it used as is.

evidolob and others added 3 commits October 13, 2020 16:00
@evidolob evidolob requested a review from JPinkney October 13, 2020 13:35
@evidolob
Copy link
Collaborator Author

@JPinkney I fix that, can you check again?

Copy link
Contributor

@JPinkney JPinkney left a comment

Choose a reason for hiding this comment

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

Tested again and now everything seems to be working as expected!

@evidolob evidolob merged commit c36aab6 into redhat-developer:master Oct 15, 2020
@evidolob evidolob deleted the add-schema-to-diagnostics branch October 15, 2020 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problems and suggested solutions should be marked with plugin-name Add schema info to diagnostics
4 participants