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

Module Markdown Optional Fields #334

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Nalum
Copy link
Contributor

@Nalum Nalum commented Feb 2, 2024

Based on feedback in cue-lang/cue#2783 this PR updates the Markdown document generation to pull the optional fields into the table data. This is done via the cue.Value.Fields() iterator.

@Nalum Nalum marked this pull request as draft February 2, 2024 12:36
@Nalum Nalum force-pushed the module-md-optional-fields branch 2 times, most recently from 238c3e6 to c01dd54 Compare February 2, 2024 15:02
@Nalum Nalum force-pushed the module-md-optional-fields branch from b4ba461 to d6d05d7 Compare March 3, 2024 11:31
@Nalum
Copy link
Contributor Author

Nalum commented Mar 27, 2024

@stefanprodan with some feedback from the cue team I've gotten this so it now will print the definition of the field, see the README for the blueprints starter module. I want to refine the module so that name and namespace are not output, but wanted to bring it to your attention.

@Nalum
Copy link
Contributor Author

Nalum commented Mar 27, 2024

I'm still not getting what I want from the labels and annotations though :/

@Nalum Nalum marked this pull request as ready for review October 3, 2024 11:21
@Nalum Nalum marked this pull request as draft October 3, 2024 15:48
@Nalum Nalum marked this pull request as ready for review October 3, 2024 16:34
@Nalum
Copy link
Contributor Author

Nalum commented Oct 3, 2024

I have pulled and rebased the latest main, this behaves in the same way. I think the changes here are worth merging, I'll spend some time digging into cue releases since the initial work on this and see if I can get the result I initially wanted for annotations and labels.

@stefanprodan
Copy link
Owner

Hey @Nalum with this we remove the default column from the readme, would be possible to preserve it or we need to merge type with defaults? If so, I think we need to rename the type column to something else.

@Nalum
Copy link
Contributor Author

Nalum commented Oct 4, 2024

I removed it because the type information have use the type and the default in one result, I had not found a way to get both separately at the time, maybe there is a way now, I'll dig into that too. scratch that, it was doing this, I can't recall the reason for making that change now.

Do you want to hold off on merging this for now so?

@Nalum
Copy link
Contributor Author

Nalum commented Oct 4, 2024

Ah I think the goal was to show the cue type information as is. For example if we just had the type and default this would not show you the possible values:

| `test: startupAPICheck: service: type:` | `*"ClusterIP" \| "NodePort" \| "LoadBalancer" \| "ExternalName"` |

You would have this instead:

| `test: startupAPICheck: service: type:` | `string` | `ClusterIP` |

@Nalum
Copy link
Contributor Author

Nalum commented Oct 4, 2024

I guess this adds a requirement to understand cue syntax 🤔

Refactor code so we are not passing in values and only using the defaults from the module config

Show parent field if there are no child fields to show

We want to show the labels field (among others) in the markdown table if
so that it is part of the documentation rather than showing all the
preset labels as individual fields in the docs.

Update schema with +nodoc tags and copy to blueprint starter module

Hide certain fields

This change brings the output more inline with the definition

Update output to remove default column

Commit updated test data

Update nodoc behaviour

Update structure to accomodate nodoc

Signed-off-by: Luke Mallon (Nalum) <[email protected]>
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.

2 participants