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

DOC-252 Fix broken mermaid diagrams in docs #5467

Closed
wants to merge 7 commits into from

Conversation

neverett
Copy link
Contributor

@neverett neverett commented Jun 11, 2024

Why are the changes needed?

Closes #4864

What changes were proposed in this pull request?

  • Update mermaid diagram format in conf.py
  • Install Node.js and npm install -g @mermaid-js/mermaid-cli during build process

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

#5373

Docs link

TK

Signed-off-by: nikki everett <nikki@union.ai>
@neverett neverett self-assigned this Jun 11, 2024
@neverett neverett requested a review from ppiegaze as a code owner June 11, 2024 16:57
Signed-off-by: nikki everett <nikki@union.ai>
@neverett neverett changed the title Fix broken mermaid diagrams in docs DOC-252 Fix broken mermaid diagrams in docs Jun 11, 2024
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.98%. Comparing base (bd2f9a8) to head (0cb167e).
Report is 150 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5467      +/-   ##
==========================================
- Coverage   61.93%   60.98%   -0.96%     
==========================================
  Files         611      793     +182     
  Lines       36463    51313   +14850     
==========================================
+ Hits        22582    31291    +8709     
- Misses      11907    17136    +5229     
- Partials     1974     2886     +912     
Flag Coverage Δ
unittests-datacatalog 69.31% <ø> (ø)
unittests-flyteadmin 58.64% <ø> (?)
unittests-flytecopilot 17.79% <ø> (ø)
unittests-flytectl 67.97% <ø> (ø)
unittests-flyteidl 79.04% <ø> (ø)
unittests-flyteplugins 61.84% <ø> (ø)
unittests-flytepropeller 57.32% <ø> (ø)
unittests-flytestdlib 65.82% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@davidmirror-ops davidmirror-ops left a comment

Choose a reason for hiding this comment

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

Thank you!

@neverett
Copy link
Contributor Author

@davidmirror-ops looks like docs tests are failing because the MMCloud plugin example page has a 404ing link. Here's a PR to remove and redirect the MMCloud plugin pages -- once it's merged, I can update the branch in this PR and the tests should pass.

Signed-off-by: nikki everett <nikki@union.ai>
Signed-off-by: nikki everett <nikki@union.ai>
Signed-off-by: nikki everett <nikki@union.ai>
@neverett
Copy link
Contributor Author

neverett commented Jun 11, 2024

@cosmicBboy I'm able to generate mermaid diagrams locally with the changes in this PR, but docs tests are failing with command '/home/docs/.asdf/shims/mmdc' cannot be run (needed for mermaid output), check the mermaid_cmd setting.

That is the location mmdc ends up installed in on the build server (see the readthedocs build logs), so I'm not sure what else to try -- any ideas?

@cosmicBboy
Copy link
Contributor

As far I as I can tell the mmdc path will be different in readthedocs build env from the githin actions monodocs env.

I think the monodocs build GH workflow needs to be updated:

And then in conf.py the path needs to be set as an environment variable.

However, weird thing is that even in the readthedocs preview the mermaid diagrams aren't showing up: https://flyte--5467.org.readthedocs.build/en/5467/deployment/configuration/auth_appendix.html

@cosmicBboy
Copy link
Contributor

trying out this smaller configuration change: #5498

@neverett
Copy link
Contributor Author

Closing since this was fixed in #5498

@neverett neverett closed this Jun 24, 2024
@neverett neverett deleted the nikki/docs/fix-mermaid-diagrams branch August 27, 2024 18:13
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.

[Docs] Mermaid diagrams not rendering
3 participants