crossplane / docs

Repo for Crossplane documentation.
https://docs.crossplane.io
Other
49 stars 110 forks source link

API Generator Improvements #737

Closed plumbis closed 5 months ago

plumbis commented 6 months ago

This PR provides the following updates to the existing API generation routine:

All of the files in the descriptions/ directories are just copies of the existing description fields in the CRDs.

Since the new API page pulls from the description files they had to all be included in this PR, making it super-sized.

Why build from an external file? We identified three problems with the current approach:

  1. There are many endpoints that are inherited from upstream tools like Kubernetes machinery. This means changing the description fields maybe difficult or impossible.
  2. Docs needs to be able to move faster and independent than code releases. We don't want to have to wait for a release of Crossplane to generate a new set of CRDs just to update the descriptions.
  3. OpenAPI only supports markdown inside of a description field, preventing API documentation from adding any additional feature-rich additions.

This allows the script to have upstream source build and enforce the API structure and schema while allowing docs to have more flexibility in what's place into the descriptions.

netlify[bot] commented 6 months ago

Deploy Preview for crossplane ready!

Name Link
Latest commit 96ac0a317d2a970754417cd0b04be4688ca623c2
Latest deploy log https://app.netlify.com/sites/crossplane/deploys/66030e09da87150008d36ad2
Deploy Preview https://deploy-preview-737--crossplane.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

Lighthouse
1 paths audited
Performance: 86 (🟢 up 2 from production)
Accessibility: 88 (🔴 down 3 from production)
Best Practices: 83 (no change from production)
SEO: 93 (no change from production)
PWA: 70 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

plumbis commented 6 months ago

As an exercise I updated the top level descriptions of all of the core types in crossplane/crossplane #5527.

My experience wasn't great for the following reasons:

Here's a bad code example. Notice the blank line without a comment.

// A FunctionRevision represents a revision of a Function. Crossplane
// creates new revisions when there are changes to the Function.

// Crossplane creates and manages FunctionRevisions. Function Revisions
// aren't designed for user changes.
// +kubebuilder:subresource:status
// +kubebuilder:storageversion

Here is part of the error make reviewable generates. Maybe we can fix tooling but this was hard to understand what went wrong.

1 chart(s) linted, 0 chart(s) failed
15:32:34 [ .. ] verify go modules dependencies have expected content
all modules verified
15:32:38 [ OK ] go modules dependencies verified
15:32:38 [ .. ] golangci-lint
apis/pkg/pkg.go:25:10: could not import github.com/crossplane/crossplane/apis/pkg/v1beta1 (-: # github.com/crossplane/crossplane/apis/pkg/v1beta1
apis/pkg/v1beta1/function_interfaces.go:332:12: cannot use &r (value of type *FunctionRevision) as "github.com/crossplane/crossplane/apis/pkg/v1".PackageRevision value in assignment: *FunctionRevision does not implement "github.com/crossplane/crossplane/apis/pkg/v1".PackageRevision (missing method DeepCopyObject)
apis/pkg/v1beta1/register.go:78:25: cannot use &FunctionRevision{} (value of type *FunctionRevision) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to SchemeBuilder.Register: *FunctionRevision does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)) (typecheck)
    v1beta1 "github.com/crossplane/crossplane/apis/pkg/v1beta1"
            ^
cmd/crank/xpkg/install.go:39:2: could not import github.com/crossplane/crossplane/apis/pkg/v1beta1 (-: # github.com/crossplane/crossplane/apis/pkg/v1beta1
apis/pkg/v1beta1/function_interfaces.go:332:12: cannot use &r (value of type *FunctionRevision) as "github.com/crossplane/crossplane/apis/pkg/v1".PackageRevision value in assignment: *FunctionRevision does not implement "github.com/crossplane/crossplane/apis/pkg/v1".PackageRevision (missing method DeepCopyObject)
apis/pkg/v1beta1/register.go:78:25: cannot use &FunctionRevision{} (value of type *FunctionRevision) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to SchemeBuilder.Register: *FunctionRevision does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)) (typecheck)
    "github.com/crossplane/crossplane/apis/pkg/v1beta1"
    ^
...

Maybe it's not worth making big changes in the docs (this PR) for changing a handful of API endpoint docs but the current experience isn't great.

plumbis commented 6 months ago

As a datapoint I looked at all the problematic descriptions there today and every one of them is coming from upstream Kubernetes, not from Crossplane source.

Examples:

  1. https://docs.crossplane.io/v1.15/api/#ControllerConfig-spec-volumes-hostPath
  2. https://docs.crossplane.io/v1.15/api/#Configuration-spec-packagePullSecrets-name
  3. https://docs.crossplane.io/v1.15/api/#ControllerConfig-spec-volumes-awsElasticBlockStore-fsType
  4. https://docs.crossplane.io/v1.15/api/#ControllerConfig-spec-volumes-hostPath
  5. https://docs.crossplane.io/v1.15/api/#DeploymentRuntimeConfig-spec-deploymentTemplate-spec-strategy-rollingUpdate

For (3) and (5) they contain invalid markdown in the original source (---). As we discussed we can filter these kinds of characters out but just making note of it.

plumbis commented 6 months ago

Another strike against source-first is that the authoring is for Godoc, which limits where and how markup can be put into the description comment.

https://github.com/crossplane/crossplane/pull/5527#discussion_r1543582160

jbw976 commented 5 months ago

Thanks for looking more deeply into the dev/docs experience @plumbis of what it would look like to continue taking the approach where Crossplane source code is the source of truth, it helps to understand the drawbacks that has on this docs project.

I'm trying to get to a place of comfort with taking on a full divergence of all 8K+ descriptions. Can you refresh my memory on the following few points?

Thanks @plumbis!

plumbis commented 5 months ago

The challenge with diffs/automation is "who's the source of truth"? Specifically, how do we know what is an import vs what is an update?

For example, If we import descriptions to the docs and then change the source code. In the scenario we know the source should be the definitive description, but what do we do when automating the docs update?

Since we aren't working on the same files we can't really do a native diff.

Regarding the question on an overlay, we can absolutely do this. The challenge will be creating the right file path to override a description. For example, [https://github.com/plumbis/crossplane-docs/blob/api-generator/content/v1.15/api/descriptions/apiextensions.crossplane.io/CompositionRevision/properties/spec/properties/patchSets/items/properties/patches/items/properties/fromFieldPath/description.yaml](CompositionRevision spec.patchSets.patches.properties.fromFieldPath) is a pretty gnarly path.

IMO we either have to have it in the description YAML mapping a key to a description or have the file path (like this example) provide the full resource path.

I'm open to other ideas for solving it.

jbw976 commented 5 months ago

I was probably thinking of something less sophisticated and more naive @plumbis. I was imagining for the vast majority of API types/fields we would just be using the crossplane source. In a much smaller subset that we consciously want to diverge with, we create an override file for each description with whatever content we want. From that point forward, we have diverged that field and there isn't any automatic updating or merging over time with Crossplane source code updates.

Obviously I have no idea how to implement things in this docs repo, but the idea I had would be:

would something like that be possible? no intelligent merging and source of truth selection, just a one time conscious decision to diverge a field with an override and then we take on the burden of maintaining just those overrides manually over time.

This may not be possible and it could be a terrible idea given our tools/stack, but I'm hoping to consider it at least from the angle that it would give us:

plumbis commented 5 months ago

Given some upcoming changes to docs maintainers coming up I'm going to close this out and I believe we should stick with descriptions from source as the method for API documentation at this time.

I'll open another PR for the infra improvements that are in this PR but not directly related to the description field.