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

Bump Docsy to 0.6.x #48724

Closed
wants to merge 3 commits into from
Closed

Conversation

sftim
Copy link
Contributor

@sftim sftim commented Nov 14, 2024

This PR upgrades to a newer version of Docsy: 0.6.0 (preview)

Relevant to issue #44002

In the testing I've done, I haven't found any different look or behavior that still needs fixing.

/area web-development

This PR incorporates the commits from PRs:

@k8s-ci-robot k8s-ci-robot added area/web-development Issues or PRs related to the kubernetes.io's infrastructure, design, or build processes language/en Issues or PRs related to English language labels Nov 14, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Nov 14, 2024
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 14, 2024
Copy link

netlify bot commented Nov 14, 2024

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit d06130c
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/677fc37deb5c0c00083b2469
😎 Deploy Preview https://deploy-preview-48724--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@Arhell Arhell left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: ce181c72da3a99a11aa14ca37cb7a32c391acd56

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2024
@sftim sftim force-pushed the 20241102_docsy_zero_six branch from c7d6601 to 76b7102 Compare November 18, 2024 14:17
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 18, 2024
@k8s-ci-robot k8s-ci-robot requested a review from Arhell November 18, 2024 14:17
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 18, 2024
@sftim sftim force-pushed the 20241102_docsy_zero_six branch from 76b7102 to 76b7ce8 Compare November 18, 2024 20:36
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2024
@sftim sftim mentioned this pull request Nov 20, 2024
@sftim
Copy link
Contributor Author

sftim commented Nov 20, 2024

Hmm, I think this needs a new approach.
/close

I hope to get time to revisit it.

@k8s-ci-robot
Copy link
Contributor

@sftim: Closed this PR.

In response to this:

Hmm, I think this needs a new approach.
/close

I hope to get time to revisit it.

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 the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 9, 2025
@sftim
Copy link
Contributor Author

sftim commented Jan 9, 2025

Before adding LGTM, see #48724 (comment)

@chalin
Copy link
Contributor

chalin commented Jan 9, 2025

Hmm. Not sure that https://deploy-preview-48724--kubernetes-io-main-staging.netlify.app/docs/contribute/style/diagram-guide/#example-2-ingress is rendering right after my changes.

It appears to be rendering properly now:

image

What appears off to you?

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

@sftim - can you also fix content/zh-cn/docs/contribute/style/diagram-guide.md by changing ```mermaid to ```text, since it requires the same fix as the English page: https://deploy-preview-48724--kubernetes-io-main-staging.netlify.app/zh-cn/docs/contribute/style/diagram-guide/#%E7%A4%BA%E4%BE%8B-2-ingress

@sftim
Copy link
Contributor Author

sftim commented Jan 9, 2025

@sftim - can you also fix content/zh-cn/docs/contribute/style/diagram-guide.md by changing ```mermaid to ```text, since it requires the same fix as the English page: https://deploy-preview-48724--kubernetes-io-main-staging.netlify.app/zh-cn/docs/contribute/style/diagram-guide/#%E7%A4%BA%E4%BE%8B-2-ingress

I was going to check with the Chinese localization team about how they want to cover this.

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Pending resolution of #48724 (comment), which may or may not be in scope of this PR (if not, an issue should be created to keep track of the required change), LGTM

/cc @nate-double-u

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chalin, lhajouji, salaxander

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

The pull request process is described 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

@chalin
Copy link
Contributor

chalin commented Jan 9, 2025

Btw, my previous LGTM of this PR is for the minimal changes it introduced in support of bumping up the version of Docsy to 0.6.x -- this is done is in the "minimalist" spirit of #44002. In particular, the Docsy 0.6.x Mermaid enhancements are not being integrated here. I'm assuming that this will be done later in scope of #41171.

@sftim
Copy link
Contributor Author

sftim commented Jan 9, 2025

We're all for minimal useful changes and small reviewable PRs.

@sftim
Copy link
Contributor Author

sftim commented Jan 9, 2025

After this merges, let's try to tidy up our use of Mermaid and align it to what upstream Docsy recommends.

@nate-double-u
Copy link
Contributor

nate-double-u commented Jan 10, 2025

@sftim @nate-double-u - does make container-image work for you?

Yes, it does (going through the same submodule management process I had to go through for your earlier PR (#48812))

make module-init # this will throw an error

pushd api-ref-generator && git fetch --unshallow && popd

make module-init # this will work now we've done the full clone

make container-image

make container-serve

@SayakMukhopadhyay
Copy link
Contributor

A few points and nits

  1. Do we need to set text blocks as ```text ? Just keeping ``` should be enough. But I can understand if its for verbosity.

Hmm. Not sure that https://deploy-preview-48724--kubernetes-io-main-staging.netlify.app/docs/contribute/style/diagram-guide/#example-2-ingress is rendering right after my changes.

  1. @sftim The mermaid blocks were not rendering for me, either in container or in a local build. I got the following error
    image

Failed to find a valid digest in the 'integrity' attribute for resource 'http://localhost:1313/js/mermaid-10.6.1.min.js' with computed SHA-384 integrity 'toDgDinNQVuBBJdT9Q9AnvM4szGg0g7BSEy/yLPRkkQylpeePcz8Mx/JLOFf33o+'. The resource has been blocked.

It seems like the issue lies in

<script src="{{ .RelPermalink }}" integrity="sha384-+NGfjU8KzpDLXRHduEqW+ZiJr2rIg+cidUVk7B51R5xK7cHwMKQfrdFwGdrq1Bcz"></script>
which uses a wrong integrity hash. Changing the hash to the one expected by the browser gets the mermaid diagrams to render properly. The tricky questions are:

  1. How was this working until now?
  2. Why does the netlify deployment still work whereas locally the issue crops up.
  3. Is it only happening on my system or is this reproducible?

Apart from this I have this related question (#48724 (comment)) but otherwise the PR is fine.

@sftim sftim marked this pull request as draft January 13, 2025 11:00
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 13, 2025
@sftim
Copy link
Contributor Author

sftim commented Jan 13, 2025

I think the Mermaid part might need more work. That's a shame. C'est la vie.

@SayakMukhopadhyay
Copy link
Contributor

I think the Mermaid part might need more work. That's a shame. C'est la vie.

You can get the mermaid to work by changing the integrity hash in the mermaid library that is getting imported. At least until we remove the shortcode and convert all mermaid diagrams to use code fencing.

@sftim
Copy link
Contributor Author

sftim commented Jan 13, 2025

@SayakMukhopadhyay if you'd like to, you're very welcome to take over this PR - feel free to make your own local branch and cherry pick any useful commit from this one.

I've more K8s WIP than I've capacity to look after; I've been leading on the Docsy upgrade partly because at the time nobody else was working on it. I get the sense you could easily get this one over the line; also, if you send in a PR and it looks good, I can LGTM it.

@SayakMukhopadhyay
Copy link
Contributor

@SayakMukhopadhyay if you'd like to, you're very welcome to take over this PR - feel free to make your own local branch and cherry pick any useful commit from this one.

I've more K8s WIP than I've capacity to look after; I've been leading on the Docsy upgrade partly because at the time nobody else was working on it. I get the sense you could easily get this one over the line; also, if you send in a PR and it looks good, I can LGTM it.

Yeah, I can give it a go but I hope I am not stepping on any toes.

@sftim
Copy link
Contributor Author

sftim commented Jan 13, 2025

These toes are made for steppin'

@sftim
Copy link
Contributor Author

sftim commented Jan 13, 2025

Closing in favor of #49416

/close

@k8s-ci-robot
Copy link
Contributor

@sftim: Closed this PR.

In response to this:

Closing in favor of #49416

/close

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.

@chalin
Copy link
Contributor

chalin commented Jan 13, 2025

@sftim et all: why was this PR closed? It was a valid and fully functional update to Docsy 0.6.x, with Mermaid diagrams correctly rendering, ready to be merged.

It was also understood that any further Mermaid compatibility issues would be handled in a followup PR, as Tim wrote:

After this merges, let's try to tidy up our use of Mermaid and align it to what upstream Docsy recommends.

@sftim
Copy link
Contributor Author

sftim commented Jan 13, 2025

When I checked this, I wasn't seeing the Mermaid diagrams rendering correctly across all browsers; some pages did look OK in some browsers, but that wasn't enough for me to think it was good to merge. Also, I'm very happy for @SayakMukhopadhyay - or anyone else - to pick up the baton.

We need to at least not break Mermaid vs. the existing experience.

@chalin
Copy link
Contributor

chalin commented Jan 14, 2025

@sftim - can you also fix content/zh-cn/docs/contribute/style/diagram-guide.md by changing ```mermaid to ```text, since it requires the same fix as the English page: https://deploy-preview-48724--kubernetes-io-main-staging.netlify.app/zh-cn/docs/contribute/style/diagram-guide/#%E7%A4%BA%E4%BE%8B-2-ingress

Will follow through with:

now that this PR has been closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/web-development Issues or PRs related to the kubernetes.io's infrastructure, design, or build processes cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants