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

[TechDocs] Enhance PlantUML support in the techdocs container to include external puml or pu files #63

Conversation

lavanya-sainik-ericsson
Copy link
Contributor

I have created a pull request for the issue I have outlined on backstage. Please refer here : backstage/backstage#20184.

awanlin As discussed in the issue thread, looping you here as well to have a look.

Copy link
Collaborator

@awanlin awanlin left a comment

Choose a reason for hiding this comment

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

Thanks again for this contribution @lavanya-sainik-ericsson, just left a couple of quick changes 👍

Dockerfile Outdated
@@ -26,6 +26,11 @@ RUN pip install --upgrade pip && pip install mkdocs-techdocs-core==1.2.2
# error (OSError: [Errno 8] Exec format error: 'plantuml') by using the
# following RUN command instead:
# RUN echo '#!/bin/sh\n\njava -jar '/opt/plantuml.jar' ${@}' >> /usr/local/bin/plantuml

# When adding TechDocs with plantUml diagrams, to refer external puml or pu files in any markdown file,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# When adding TechDocs with plantUml diagrams, to refer external puml or pu files in any markdown file,
# When adding TechDocs with PlantUML diagrams, to refer external puml or pu files in any markdown file,

Dockerfile Outdated
@@ -26,6 +26,11 @@ RUN pip install --upgrade pip && pip install mkdocs-techdocs-core==1.2.2
# error (OSError: [Errno 8] Exec format error: 'plantuml') by using the
# following RUN command instead:
# RUN echo '#!/bin/sh\n\njava -jar '/opt/plantuml.jar' ${@}' >> /usr/local/bin/plantuml

# When adding TechDocs with plantUml diagrams, to refer external puml or pu files in any markdown file,
# eg. '!include <referencedFileName.puml>', need to include diagrams directory eg. docs in the classpath.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# eg. '!include <referencedFileName.puml>', need to include diagrams directory eg. docs in the classpath.
# eg. '!include <referencedFileName.puml>', you'll need to include the diagrams directory eg. docs in the classpath.

…ude external puml or pu files

Issue #20184

Signed-off-by: Lavanya Sainik <lavanya.sainik@ericsson.com>
@lavanya-sainik-ericsson
Copy link
Contributor Author

Hello awanlin,

Thanks for your comments. Have done those changes. Let me know if any more corrections is needed.

Copy link
Collaborator

@awanlin awanlin left a comment

Choose a reason for hiding this comment

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

Looks good to me @lavanya-sainik-ericsson, you'll need someone from @backstage/techdocs-maintainers to review and approve, they will also do the merge

@lavanya-sainik-ericsson
Copy link
Contributor Author

lavanya-sainik-ericsson commented Oct 4, 2023

Hey awanlin,

Thanks a lot for your feedback and approval. Sure can you share how to request reviews from @backstage/techdocs-maintainers here & request their attention & valuable feedback as here I cannot see @backstage/techdocs-maintainers tagged but they are automatically tagged in other PR backstage/backstage#20229 or not sure if I am missing anything ? Let me know your views here whenever you get time :).

Copy link
Contributor

@johnphilip283 johnphilip283 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 for the change - approved!

@johnphilip283 johnphilip283 merged commit cfa8832 into backstage:main Oct 11, 2023
1 check passed
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.

3 participants