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

Autogenerate CRD docs #2116

Merged
merged 22 commits into from
Mar 6, 2022
Merged

Autogenerate CRD docs #2116

merged 22 commits into from
Mar 6, 2022

Conversation

super-harsh
Copy link
Collaborator

@super-harsh super-harsh commented Feb 25, 2022

Closes #1715

What this PR does / why we need it:

This PR adds support to generate API docs for ASOv2 CRDs for the end-users. API docs will help the end-users to get a better understanding of the fields and values for CRDs.

Special notes for your reviewer:

This PR includes the generated API docs, task and script to generate the API docs. Docs are generated separately for each group and version. The tool we're using to generate the docs is gen-crd-api-reference-docs.

How does this PR make you feel:
gif

If applicable:

  • this PR contains documentation

@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2022

Codecov Report

Merging #2116 (e9fb5ef) into main (270280c) will decrease coverage by 0.00%.
The diff coverage is 11.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2116      +/-   ##
==========================================
- Coverage   56.99%   56.98%   -0.01%     
==========================================
  Files         585      585              
  Lines      106379   106387       +8     
==========================================
  Hits        60629    60629              
- Misses      38276    38284       +8     
  Partials     7474     7474              
Impacted Files Coverage Δ
.../generator/internal/astmodel/package_definition.go 7.33% <0.00%> (-0.59%) ⬇️
...2/tools/generator/internal/config/configuration.go 19.87% <ø> (ø)
...internal/codegen/pipeline/export_generated_code.go 6.25% <25.00%> (ø)
...tools/generator/internal/codegen/code_generator.go 90.44% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 270280c...e9fb5ef. Read the comment docs.

docs/hugo/config.toml Outdated Show resolved Hide resolved
docs/api/template/config.json Outdated Show resolved Hide resolved
scripts/gen-api-docs.sh Outdated Show resolved Hide resolved
scripts/gen-api-docs.sh Outdated Show resolved Hide resolved
buf := &bytes.Buffer{}
err := groupVersionFileTemplate.Execute(buf, pkgDef)
defer buf.Reset()
Copy link
Member

Choose a reason for hiding this comment

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

What's the point of doing this given that buf goes out of scope when the function ends?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will dealloc the resources and garbage collection efficiency will be better.

Copy link
Member

Choose a reason for hiding this comment

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

Really? If you look at the Reset() implementation:

// Reset resets the buffer to be empty,
// but it retains the underlying storage for use by future writes.
// Reset is the same as Truncate(0).
func (b *Buffer) Reset() {
	b.buf = b.buf[:0]
	b.off = 0
	b.lastRead = opInvalid
}

You can see there's no actual space savings here: https://go.dev/play/p/zXGZkI8C3Rv. Even if this did actually deallocate space, I don't think there's anything that makes a difference between waiting til the GC comes to clean up the slice whose space you deallocated, vs waiting for the GC to come clean up the buf itself. AFAIK both will take the same amount of time. @Porges might know something I don't there though.

v2/tools/generator/internal/astmodel/package_definition.go Outdated Show resolved Hide resolved
GROUPNAME=$(basename $(dirname $package))

#Filter the main CRD packages
if [[ $PACKAGE_VERSION =~ $PATTERN ]] && [[ "$PACKAGE_VERSION" != *"storage" ]]
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually needed now given the storage packages also do not have doc.go? (Not entirely sure what doc.go gets the tool so just checking)

Copy link
Collaborator Author

@super-harsh super-harsh Mar 2, 2022

Choose a reason for hiding this comment

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

This checks saves us time in generating docs, if the tool goes through the storage packages then it tries to iterate through all the elements inside the package and then come up with an error no API packages found in ./api/batch/v1alpha1api20210101storage which takes a few(5-10) seconds.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe leave a comment explaining that here?

docs/api/template/config.json Outdated Show resolved Hide resolved
Harshdeep Singh and others added 5 commits March 2, 2022 16:08
…arameter in scripts/gen-api-docs.sh; Add gen-crd-api-reference-docs installation inside .devcontainer/install-dependencies.sh; Add genruntime external reference for gen-crd-api-reference-docs in template/config.json
Copy link
Member

@matthchr matthchr left a comment

Choose a reason for hiding this comment

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

This looks good to me. There are still some rough edges, for example the fact we generate documentation for the ARM types. It might be worth filing an item to stop doing that (we should just document the k8s types), but I think this is a good starting point so going ahead and approving.

@@ -62,6 +62,9 @@ go get \
go install -tags extended github.com/gohugoio/hugo@v0.88.1
go install github.com/wjdp/htmltest@v0.15.0

# for api docs
go install github.com/ahmetb/gen-crd-api-reference-docs@v0.3.1-0.20220223025230-af7c5e0048a3
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Missing TODO/note to update this when a new release is tagged?

GROUPNAME=$(basename $(dirname $package))

#Filter the main CRD packages
if [[ $PACKAGE_VERSION =~ $PATTERN ]] && [[ "$PACKAGE_VERSION" != *"storage" ]]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe leave a comment explaining that here?

err = emitGroupVersionFile(p, outputDir)
if err != nil {
return 0, err
}

// If package path does not include crossplane api dir and is not Storage package, then we generate doc file for the package
Copy link
Member

Choose a reason for hiding this comment

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

minor: Fix comment as crossplane is not longer explicitly checked here

buf := &bytes.Buffer{}
err := groupVersionFileTemplate.Execute(buf, pkgDef)
defer buf.Reset()
Copy link
Member

Choose a reason for hiding this comment

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

Really? If you look at the Reset() implementation:

// Reset resets the buffer to be empty,
// but it retains the underlying storage for use by future writes.
// Reset is the same as Truncate(0).
func (b *Buffer) Reset() {
	b.buf = b.buf[:0]
	b.off = 0
	b.lastRead = opInvalid
}

You can see there's no actual space savings here: https://go.dev/play/p/zXGZkI8C3Rv. Even if this did actually deallocate space, I don't think there's anything that makes a difference between waiting til the GC comes to clean up the slice whose space you deallocated, vs waiting for the GC to come clean up the buf itself. AFAIK both will take the same amount of time. @Porges might know something I don't there though.

@@ -53,6 +53,8 @@ type Configuration struct {
// SamplesURL is the URL the samples are accessible at. Paths will be appended to the end of this to
// build full sample links. If this is not specified, no samples links are generated.
SamplesURL string `yaml:"samplesUrl"`
// EmitDocFiles is used as a signal to create doc.go files for packages
Copy link
Member

Choose a reason for hiding this comment

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

minor: Consider saying "the default is false" (even though it's sorta obvious because this is a non-ptr boolean).

@super-harsh super-harsh merged commit 79b8124 into main Mar 6, 2022
@super-harsh super-harsh deleted the feature/gen-api-docs branch March 6, 2022 22:58
@matthchr matthchr added this to the codegen-beta-0 milestone Mar 7, 2022
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.

Autogenerate CRD docs
3 participants