Release candidate

[charith-elastic]

Candidate for first public release

[anyasabo]

This can probably be simpler -- maybe just "Generates API reference documentation for Kubernetes Custom Resource Definitions"?

[charith-elastic]

I feel like the original sentence describes how the tool works without any ambiguity. It is intended for a technical audience after all.

[anyasabo]

Do we want to ignore these? The upstream reference docs include status etc

Also it may not be necessary right now but it would be helpful to comment what the various config options are

[charith-elastic]

Yes, I noticed that the upstream docs include status fields but I don't see the usefulness of them because they are read-only fields. Including the status types pulls in a lot of additional type declarations to the docs and creates noise as well.

[anyasabo]

I think even read only fields are useful -- I've definitely looked at the reference docs to see what I can expect in status types. I'm good with waiting for later if it makes it more complicated now though.

[anyasabo]

We're running 1.17 deps now so we may want to update this as well. It looks like we use this to generate the links to the upstream docs, correct? i wonder if we can parse the correct version from the go.mod rather than relying on having it set here too. Definitely not a blocker though.

[charith-elastic]

This is configurable by users. When we use it on ECK we can set it to 1.17 as you suggest. I think it might be overkill to try and parse the Kubernetes version from go.mod as it is of very minor significance and changes rarely anyway.

[sebgl]

It looks quite clean (and also quite complex but that's life), I need to spend more time reviewing the details.

It would be nice if we could include a basic high-level test based on a dummy CRD? It would allow users to look at both the sample CRD and the generated ascii output to better understand what happens. Also would help catching regressions. WDYT?

[charith-elastic]

It would be nice if we could include a basic high-level test based on a dummy CRD?

I did consider doing it. My main concern was that it would be quite brittle and hard to maintain.

Option 1 would be to compare the generated internal data structure against a "golden" version. The question is how to represent the golden version in code or as an input file. Since the data structure contains types from Go's internal libraries, it is quite difficult to serialize/deserialize and compare those.

Option 2 would be to compare the generated output (ASCIIDoc in this first iteration). Again, that's a lot of textual content to compare and even a minor modification to the templates will break the test. The developer will have to either manually create the expected output file (tedious, error-prone work) or use the generator to produce some output, visually check it for any obvious issues, and then use that as the golden file -- which kind of defeats the purpose of an automated test.

If you have any ideas about writing a better test, I would be very interested in hearing those.

[sebgl]

... which kind of defeats the purpose of an automated test

I had option 2 in mind. We would have a dummy CRD in a test directory, run the generator against it with some default parameters, and compare the resulting asciidoc output with one we pre-generated. That pre-generated output is also stored in the test directory. (Is it called a smoke test?)

Of course it sounds a bit stupid because the pre-generated asciidoc actually results from using the code we want to test 😄

My point is not to test the actual code in this PR. I think the smoke test would bring:

• an example of what the project actually does ("see that CRD, the generator can produce that asciidoc")
• a better-than-nothing way to detect regressions in the future: if we change something in the code that should impact the output, it's expected the PR with code changes also contains a change in the pre-generated test output. If the code change is not supposed to modify the output, but actually does, the automated smoke test would most probably catch it. It's also good to easily understand the impact of future PRs: if the PR is supposed to add eg. a whitespace in one place, we'll notice it pretty clearly in the generated test output.

[sebgl]

Forgot to mention in the comment above: if we end up deciding a test would be nice, I'm totally fine with doing it later in a follow-up PR.

[sebgl]

🚀 This is great work!
I left a few comments for minor things, I'm fine with merging in the current state. LGTM!

[charith-elastic]

Forgot to mention in the comment above: if we end up deciding a test would be nice, I'm totally fine with doing it later in a follow-up PR.

👍 I will think a bit more about how to effectively test this and follow up in a separate PR.