-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 a resource reference generator #50515
base: master
Are you sure you want to change the base?
Conversation
@ptgott - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes. |
🤖 Vercel preview here: https://docs-771hg03b4-goteleport.vercel.app/docs |
Reviewer note: this PR doesn't include generated docs, since that would make the diff even larger than it is now. |
145bf35
to
04d814b
Compare
Amplify deployment status
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable, I'll have to test it out soon!
build.assets/tooling/cmd/resource-ref-generator/reference/reference.go
Outdated
Show resolved
Hide resolved
build.assets/tooling/cmd/resource-ref-generator/resource/resource.go
Outdated
Show resolved
Hide resolved
build.assets/tooling/cmd/resource-ref-generator/reference/reference.go
Outdated
Show resolved
Hide resolved
build.assets/tooling/cmd/resource-ref-generator/reference/reference.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass looks generally good - glad to see this being worked on!
I think my major concern is there may be parts of this generator that expect/rely on the resource being an old-style resource prior to RFD153 that is generated by GoGo Protobuf. I'd recommend possibly including a new RFD153-compliant resource in your testdata to ensure it'll work with the newest stuff (e.g the Bot resource is one of these)
build.assets/tooling/cmd/resource-ref-generator/reference/testdata/src/app.go
Outdated
Show resolved
Hide resolved
Thanks for the feedback! I've pushed a change in e8b2609 to support RFD153-compliant resources. The biggest issue I had here was that there doesn't seem to be a consistent way to assign the |
build.assets/tooling/cmd/resource-ref-generator/reference/reference.go
Outdated
Show resolved
Hide resolved
build.assets/tooling/cmd/resource-ref-generator/reference/reference.go
Outdated
Show resolved
Hide resolved
8802f44
to
04ab4e4
Compare
04ab4e4
to
5ae21e5
Compare
5ae21e5
to
d1908d5
Compare
fd6d672
to
7e8fe24
Compare
7e8fe24
to
5ae5135
Compare
5ae5135
to
ed13435
Compare
ed13435
to
662f465
Compare
662f465
to
daed781
Compare
daed781
to
41c4c27
Compare
41c4c27
to
a1f5ed0
Compare
a1f5ed0
to
c9bb2c5
Compare
Closes #16948 Closes #9949 Implements RFD 130. See the included README for information about how the generator works. This implementation breaks from RFD 130 in the following ways: - The reference includes separate pages for dynamic resources in order to make it easier to read. - The generator specifies the kind and version of each dynamic resource. To do this, the generator looks up a method called for each resource type to assign the type's version and kind. - The generator uses a YAML configuration file. This is because the config ended up including more fields than specified in the RFD, so separating the config from the source made sense to give users less noise to deal with. - In the example YAML blocks and type table values, leave types empty if we can't process them. Edit the RFD: - Be less specific about the output template so this isn't wedded to implementation details. - Remove the description of example YAML delimiters. These are no longer viable, as we maintain several documentation generators that extract comments from the source.
RFD 153 resource types include unexported fields and include a `Metadata` field from a different package than legacy resources types. They also include the `Metadata` field with a pointer. Accommodating the different `Metadata` field only requires adding another entry to `required_field_types` in the config. This change also adjusts the generator to ignore unexported fields in struct declarations, and to treat pointer field types the same way as value field types. Finally, this change removes some unnecessary spacing from the reference template.
- Embed the reference page template - Fix comments - Fix config path in main - Use filepath.Join instead of path.Join - Organize go.mod properly - Move template parsing into an init function - Change a var declaration to a const
- Update copyright year. - Wrap errors properly in `fmt.Errorf`.
Enclose curly braces in backticks to prevent docs builds from breaking.
c9bb2c5
to
c9563c1
Compare
$ cd build.assets/tooling/cmd/resource-ref-generator | ||
$ go run . -config config.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a make target to the root makefile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, the root Makefile has targets for checking that auto-generated docs are up to date (here), but not for generating the docs. I could follow this convention and put the target in build.assets/Makefile
, though a comment in the file says this is for producing official Teleport releases. At the same time, a root make target for running the docs generators would be nice, since they're spread throughout the source.
Come to think of it, is build.assets/tooling/cmd
the best place for this project? Would it make sense to place it in docs
?
|
||
{{ .Resource.Description }} | ||
|
||
{/*Automatically generated from: {{ .Resource.SourcePath}}*/} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move this line to the top of the file and make it a bit louder so people don't unintentionally try to edit this file?
I'm not sure if gopls operates on .tmpl files, but if it does there's a standard "code generated" comment that will actually tell editors to warn users who try to edit generated files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved the line to the top of each generated file, but wasn't able to find details about standard "code generated" comments that cause editor warnings. I've seen "DO NOT EDIT" used a lot, so I've added that text.
build.assets/tooling/cmd/resource-ref-generator/reference/reference_test.go
Outdated
Show resolved
Hide resolved
build.assets/tooling/cmd/resource-ref-generator/reference/reference_test.go
Outdated
Show resolved
Hide resolved
build.assets/tooling/cmd/resource-ref-generator/reference/testdata/src/app.go
Outdated
Show resolved
Hide resolved
- Use standard library packages instead of afero for storing generated files in tests. As part of this work, change the `SourcePath` in `ReferenceEntry` to use a directory relative to the project root, meaning that we don't need to include a temporary directory path in test expectations. Also clarify some errors in `Generate` that were returned while debugging in-test directory management. - Remove unnecessary code to find the caller of `TestGenerate`. - Remove unnecessary package alias. - Move auto-generation warning comments to the top of each generated page. (These were formerly at the top of each section, since an earlier iteration attempted to generate a single reference for all resources.)
Closes #16948
Closes #9949
Closes #39564
Implements RFD 130.
See the included README for information about how the generator works.
This implementation breaks from RFD 130 in the following ways:
Edit the RFD: