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 Crossplane package language server xpls to up as a subcommand #129

Merged
merged 81 commits into from
Jan 20, 2022

Conversation

tnthornton
Copy link
Member

@tnthornton tnthornton commented Jan 19, 2022

Description of your changes

We've found that a common pain point for package authors is the lack of feedback they receive during the author loop (write yaml -> test if yaml is valid -> write more yaml). In order to make initial strides to address that pain and lower the barrier to entry for new authors into the package space, we're adding a Crossplane package (xpkg) language server to the up CLI.

The initial alpha release of xpls includes the following features:

  • crossplane.yaml validations
    • validates that the defined package (name and version) exist locally. Returns an error diagnostic if validation fails.
    • validates that the defined package type matches the package itself. Returns an error diagnostic if validation fails. For example, if the author specifies:
      spec:
          dependsOn:
              - configuration: provider-aws
                 version: "v0.20.0"
      an error diagnostic will be returned due to provider-aws being a Provider package, not a Configuration package.
  • validation of composed resource definitions per the resource's CRD's openAPIV3Schema
    • a differentiating feature from the general Kubernetes plugin (in VSCode) is that Compositions have embedded resources, unfortunately, the CRD for a Composition defines spec.resources[*].base as a general object. xpls solves this by using the openAPIV3Schema for the corresponding composed resource to validate its fields.
    • for the alpha release, xpls will highlight areas where the current definition does not meet the spec requirements.
  • establishing a local cache for external package contents

In addition to the above features for xpls, this changeset includes a new xpkg subcommand dep.
Using up xpkg dep you can do the following:

  • up xpkg dep invoked in a current package repo will install the package dependencies defined in the package's crossplane.yaml
  • up xpkg dep <package-name> will install the supplied package into the local cache. If the command is invoked within a package repo the supplied package will be added to the crossplane.yaml for the package.
    Example invocations:
    # this invocation will add the latest version of crossplane/provider-aws to the local cache
    up xpkg dep crossplane/provider-aws
    
    # this invocation will add specifically version v0.20.0 of crossplane/provider-aws to the local cache
    up xpkg dep crossplane/provider-aws@v0.20.0
  • up xpkg dep -c will clear the local cache

I have:

  • Read and followed Upbound's contribution process.
  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Beyond unit tests, the code was tested performing validations against a repo that contains an xpkg modeled after getting-started-with-aws as well as various internally authored packages.

tnthornton and others added 30 commits January 18, 2022 15:40
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Adds a simple read write closer stdio transport such that the language
server can communicate of stdout / stdin with client.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Adds an initial workspace implementation that contains a cache of nodes
and validators. Workspace parsing can either be done at the workspace or
file level. In the future we will want to support incremental parsing
using text change notifications from the LSP client. We will also want
to support reparsing nodes based on changes to nodes they depend on.
This can be accomplished by constructing a graph of all nodes (objects)
in a workspace, then traversing the edges to a node when it is updated.

Validators are currently constructured from the package cache, but
assume the on-disk representation is CRDs, rather than xpkg tarballs.
This will need to be adjusted. There is also currently only support for
v1 CRDs, but we will need to support both v1beta1 CRDs and all versions
of XRDs for validation schemas.

A workspace is thread-safe, but currently use coarse-grained locking,
which can likely be optimized in the future.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Adds an initial LSP handler, which only supports initialization and
validation on save.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Updates xpls serve to run a jsonrpc2 connection with the xpls handler
implementation. Graceful shutdown is not currently handled.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Adds --verbose flag to up xpls serve in extension.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Removes the now unused crank parser from the original LSP
implementation.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
adjusted the cache key to just be the metav1.Dependency

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
update Workspace to take func options for easier overriding in tests

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
add optional flag for specifying package type to add

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
…istence of package root

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
- refactor cache model to better decouple cache entry management and entry details
- remove --type flag in favor of getting details from package meta file

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
…guns in cache package

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Adds recursive parsing of embedded resources within a Composition. As
part of this change documents are unmarshalled to unstructured during
parsing, rather than waiting until validation. This ensures that we do
attempt to parse embedded resources when the parent Composition is not
well formed and also reduces the number of path lookups we have to
perform for acquiring GVK.

Embedded resources are declared as dependants of their parent, but exist
as standalone package nodes in the workspace. This allows for future
validation granularity, such as only validating a package node and the
resources that depend on it, which may be embedded resources, or in the
future, resources that depend on the schema defined by the resource.

Diagnostic generation from validation errors has also been separated
from the broader validation logic. A follow up will implement
diagnostics on failed parsing.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
This error is dead code as it is no longer used in parsing logic.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Updates parsing logic to continue attempting to parse nodes, documents,
and files if we encounter a preceding error. This allows us to provide
as much information as we possibly can to the user. Parsing errors are
not currently surfaced in the editor, but they absolutely should be.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Adds a note to indicate that we should be surfacing general errors if we
cannot find the exact path.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
- also fixes a performance issue with parsing project workspace

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
adjust tests to accomodate use case

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
package.ndjson has a new piece of details added to the top of the file that includes image meta data (registry, repo, version, digest)

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
…d extra info

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
… parser and ndjson parser results

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
…ier to test future

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
…fic validators

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Copy link
Contributor

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

A few comments, but otherwise this is looking pretty good (almost all of this has already undergone review) -- awesome work here @tnthornton!

docs/xpls.md Outdated
Comment on lines 14 to 18
## Install current version of `up-ls` as `up`
These steps will walk you through setting up `up` with the new features locally.

1. Clone `http:s//github.com/upbound/up-ls` locally.
2. cd into up-ls and run `make install`
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe these references to up-ls need to be updated 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few more throughout this doc as well

Makefile Outdated
Comment on lines 36 to 38
GO_PKG_DIR = $(shell echo $$GOPATH)
DEBUG = 1
GO_BUILDFLAGS = -gcflags='all=-N -l'
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want the DEBUG and GO_BUILDFLAGS set here anymore. We also likely shouldn't be overriding GO_PKG_DIR because that has impact on how we do caching as well (though we can work around it if absolutely necessary). I believe the GO_PKG_DIR bit is to make the install go to the right location -- I wonder if we could update guidance for development to just build and then make sure it is in your PATH?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good.

We also likely shouldn't be overriding GO_PKG_DIR because that has impact on how we do caching as well

I believe I added that awhile back for two reasons:

  1. to have the output of go install end up in the right location
  2. we were duplicating efforts between the go mod cache and the .cache dir in up.

Overriding that location subverts the sandboxing in our builds, totally understand that, I just wanted to provide the extra context there for anyone else coming across these settings.

docs/xpls.md Outdated
These steps will walk you through installing the VSCode extension that will
start interacting with `xpls` locally.

1. Clone https://github.com/upbound/vscode-up locally.
Copy link
Contributor

Choose a reason for hiding this comment

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

Guessing our guidance here will want to be updated to get this from the marketplace when it is available -- I could see that being a follow up if you prefer, and perhaps this section could be moved to local development documentation (rather than the docs from this dir that we sync to cloud.upbound.io/docs)

Copy link
Member Author

Choose a reason for hiding this comment

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

and perhaps this section could be moved to local development documentation

I think I'd prefer this path. Currently, publishing to the marketplace will be blocked by this PR getting merged so I'd rather not end up with a circular deployment dependency 😅 .

Makefile Outdated Show resolved Hide resolved
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants