Skip to content

Conversation

@vitorfloriano
Copy link
Contributor

This PR fixes the case in which the license would be extracted and displayed as normal text. Now the license is rendered as a code block.

Related to #4862 and #5175.

Before:

image

After:

image

The license is now displayed as code block, not as normal text.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 9, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @vitorfloriano. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 9, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vitorfloriano
Once this PR has been reviewed and has the lgtm label, please assign kavinjsir for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@camilamacedo86
Copy link
Member

The appache license I think we must to hidden 100%
That does not bring any value in the docs.

}

// skip if it's a license comment
if strings.Contains(lit, "Copyright") || strings.Contains(lit, "Licensed under") {
Copy link
Member

Choose a reason for hiding this comment

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

We need to hidden that 100%

Copy link
Contributor Author

@vitorfloriano vitorfloriano Nov 9, 2025

Choose a reason for hiding this comment

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

@camilamacedo86 I pushed a change in the logic. We first check if the code block is a license so we don't extract it, then we check if the collapsible is empty and skip it, then we put the source citation on top of the first code block that is apparent:
image

WDYT?

The program now skips the license from being extracted, then skips empty collapsible from being rendered, then puts the source citation on top of the first apparent code block.
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 9, 2025
}

if len(lit) < 3 || lit[0] != '/' || lit[1] != '*' {
return false
Copy link
Member

@camilamacedo86 camilamacedo86 Nov 9, 2025

Choose a reason for hiding this comment

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

Could we not do as in the other pr #5175

Image Image

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't this hide some collapsibles that are meant to stay? For example, we have some collapsibles in here and in here that we should keep...

Copy link
Member

Choose a reason for hiding this comment

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

All imports and license ones bring no value at all
That is why they are collapsed.
The workaround was collapse them out to hidden but we can hidden 100%

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@camilamacedo86 I double-checked #5175 and it is hiding the collapsed licenses and imports. I'm closing this PR as duplicate.

@vitorfloriano
Copy link
Contributor Author

Honestly, @camilamacedo86 I think we should rethink the whole literate extraction. It is basically trying to replicate what mdBook already does natively.

IMHO, the literate extraction system got overly complex, perhaps due to clever solutions (hacks) to work around past limitations in mdBook.

I'm planning on writing a design proposal to change how we extract code from the testdata samples using only mdBook's {{#include filename:lines}} feature and keep all the tutorial instructions in markdown files. That would make it much easier for contributors to suggest improvements to the docs, as well as to identify changes in the test samples.

mdBook newer versions have some nice features that we can't use because our custom preprocessors (literate.go and markerdocs.go) are not compatible with them. We should strive towards moving away from them and rely on mdBook's features. They are more than sufficient for our usecase and would greatly reduce the overhead to maintain the docs updated, IMO.

Due to the gargantuan size of this refactor, it could even be a project for a new mentorship! 🤩

How does that sound to you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants