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

Support for Multiple Kubernetes Schemas and CRDs #841

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

tricktron
Copy link

@tricktron tricktron commented Jan 31, 2023

What does this PR do?

This PR allows to set custom Kubernetes schema versions and Kubernetes Custom Resource Definitions (CRDs).

How Does it Work?

This PR extends #824 by introducing a new yamlSettings.kubernetesSchemaURLs setting and its corresponding notification, e.g:

{
    "yaml.kubernetesSchemaURLs": [ 
        "https://raw.githubusercontent.com/tricktron/CRDs-catalog/f-openshift-v4.11/openshift.io/v4.11/all.json",
        "https://raw.githubusercontent.com/yannh/kubernetes-json-schema/master/v1.26.1-standalone-strict/all.json" 
    ]
}

What issues does this PR fix or reference?

Is it tested? How?

  • Yes I added three tests.
  • Manually building the vscode extension and this yaml language server and then using/testing it.

@fourpastmidnight
Copy link

fourpastmidnight commented Feb 3, 2023

Why not allow one to specify, within the settings JSON under the kubernetes property, either a simple string value (currently a glob) or a map specifying the version(s) of schemas to be used?

The default configuration would be as it is today, and would simply use the latest released version of the schema. But, with the proposal above, the configuration could look like (note the syntax is using formal JSON syntax to be compatible with coc.nvim, but this could equally apply to VS Code, too, by not using quotes around the property names):

{
    "yaml.settings": {
        "kubernetes": {
            "versions": [
                "1.22.3",
                "1.22.4",
                "1.22.5"
            ],
            "files": "*.ya?ml"
        }
    }
}

NOTE
I was struggling to name the property containing the glob. I'm not married to the name, just had to put something there ;)

@gorkem
Copy link
Collaborator

gorkem commented Feb 3, 2023

I am not thrilled with the idea that a segment in the URL changes the meaning of the setting. It is prone to users unintentionally defining Kubernetes schema. I would rather have an intentional setting to manage Kubernetes schemas. Perhaps a combination of what is proposed on #824 and what @fourpastmidnight is proposing.

OTOH, I am a bit torn about Kubernetes support. Kubernetes is the only built-in schema that we have on yaml-language-server and this is due to the reasons how I had bootstrapped the project initially. Once we implemented a generic YAML LSP, the built-in Kubernetes support became an outlier.

@fourpastmidnight
Copy link

I thought it was strange that this lsp server explicitly supported Kubernetes while providing a generic mechanism for other YAML schemas. I wasn't sure if there was any appetite for removing this support, or plans for adding explicit support for other "popular"types.

I also think it would be better to remove explicit Kubernetes support as well in light of no plans for explicitly supporting other schemas in lieu of the generic mechanism. I only made the suggestion above based on the current implementation.

@tricktron
Copy link
Author

@gorkem

I am not thrilled with the idea that a segment in the URL changes the meaning of the setting. It is prone to users unintentionally defining Kubernetes schema. I would rather have an intentional setting to manage Kubernetes schemas.

I thought about this as well. I went with this approach because it is a non-breaking API change, e.g. the clients do not have to change anything. I was unable to come up with an intentional non-breaking change.

Maybe we can open a new pull request for the intentional kubernetes change in a second step?

If we document the magic kubernetes regex in the readme for now, e.g. in a separate Kubernetes support section, then I think this is good enough because allowing to specify the exact Kubernetes version is a great enhancement.

@coveralls
Copy link

coveralls commented Feb 13, 2023

Coverage Status

Coverage: 83.286% (+0.05%) from 83.241% when pulling 596b926 on tricktron:f-custom-kubernetes-schema into 762209c on redhat-developer:main.

@tricktron
Copy link
Author

@gorkem @fourpastmidnight Any updates?

tricktron added a commit to tricktron/vscode-yaml that referenced this pull request Feb 24, 2023
I want to support openshift and different kubernetes version schemas.
Therefore, I use this fork until the [upstream
pr](redhat-developer/yaml-language-server#841) is merged.
@tricktron tricktron force-pushed the f-custom-kubernetes-schema branch 2 times, most recently from fdf062e to 88fc144 Compare March 19, 2023 20:47
tricktron added a commit to tricktron/vscode-yaml that referenced this pull request Mar 19, 2023
I want to support openshift and different kubernetes version schemas.
Therefore, I use this fork until the [upstream
pr](redhat-developer/yaml-language-server#841) is merged.
@tricktron tricktron force-pushed the f-custom-kubernetes-schema branch 2 times, most recently from 4730865 to ef7c724 Compare March 24, 2023 09:29
@tricktron
Copy link
Author

@gorkem @fourpastmidnight @msivasubramaniaan I completely reworked this PR on the basis of #824. What do you think?

@tricktron tricktron changed the title Add Custom Kubernetes Schema Support Support for Multiple Kubernetes Schemas and CRDs Mar 24, 2023
Comment on lines 47 to 48
}

export namespace KubernetesSchemaURLsNotification {

Check failure

Code scanning / ESLint

Disallow custom TypeScript modules and namespaces

ES2015 module syntax is preferred over custom TypeScript modules and namespaces.
src/requestTypes.ts Fixed Show fixed Hide fixed
@JannoTjarks
Copy link

Hi, what is the status of this pr?
I would like to have this feature. We are running Kubernetes in Version 1.27. There happened some changes since the hard coded version 👀

@tricktron
Copy link
Author

Hi, what is the status of this pr? I would like to have this feature. We are running Kubernetes in Version 1.27. There happened some changes since the hard coded version 👀

I'll rebase and fix the code scanning results so that it is ready for merge again.

@tricktron
Copy link
Author

Ok. I rebased it. @gorkem @fourpastmidnight @msivasubramaniaan Could you run the build again and maybe give some feedback😃.

@tricktron
Copy link
Author

Any update @gorkem @fourpastmidnight @msivasubramaniaan?

@mosheavni
Copy link

any updates?

@tricktron
Copy link
Author

@msivasubramaniaan I have seen that you recently merged redhat-developer/vscode-yaml#912 which depends on this pull request. Could you merge this as well?

someone-stole-my-name and others added 7 commits June 5, 2024 15:24
Add a new notification to allow users to change the default Kubernetes schema on the fly.
Like this, we can support multiple custom kubernetes associated
text documents. E.g. a custom kubernetes version and multiple
custom resource definitions (CRDs).
@tricktron
Copy link
Author

@msivasubramaniaan I rebased again because I suspect that the merge of main through the GUI somehow triggered the duplicate items issue in the upload sarif stage.

If the same duplicate items issue happens again, then it might be worth to look at this similar issue: microsoft/sarif-js-sdk#86.

@PhiliNBG
Copy link

Any updates?

@mvillafuertem
Copy link

any updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants