-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
OSDOCS 13186 modify build_for_portal.py to detect images and not duplicate the entire image directory for every book #87375
Conversation
🤖 Wed Jan 22 13:46:12 - Prow CI generated the docs preview: |
353c558
to
5db5dce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great work, this is going to be a great optimzation! 🥳
Added some nit comments but also a question regarding the imagefiles array that I think needs to be addressed.
build_for_portal.py
Outdated
# ADDED 21 Jan 2025: selective processing of images | ||
# the set of file names is to be stored in imagefiles | ||
# The initial value includes images defined in attributes (to copy every time) | ||
imagefiles = {"kebab.png","app-launcher.png","red-hat-applications-menu-icon.jpg","delete.png"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we hard coding these values, and is there a risk that we miss new additions going forward? Also, are these uniform across all branches? Maybe we need to scan the attributes folder for images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed the logic more carefully and realized that the prepopulation is unnecessary as the attributes file is scanned as part of looping through the includes. Regression testing showed that if I removed the prepopulation, the only difference was the disappearance of these four images from the API books, which otherwise have no images at all; a build-test of these books using asciidoctor-pdf shows that no issues are caused by removing these images.
Yes this is going to significantly speed up Prow too. Local testing, got these results: Current script:
Updated script:
|
…icate the entire image directory for every book
5db5dce
to
db064d7
Compare
/lgtm great work. Will merge and CP as you see fit. |
@mramendi: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Some older changes need to be CPd. Working on those now. |
/cherry-pick enterprise-4.12 |
/cherry-pick enterprise-4.13 |
/cherry-pick enterprise-4.14 |
/cherry-pick enterprise-4.15 |
/cherry-pick enterprise-4.16 |
/cherry-pick enterprise-4.17 |
/cherry-pick enterprise-4.18 |
@aireilly: #87375 failed to apply on top of branch "enterprise-4.12":
In response to this:
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. |
@aireilly: #87375 failed to apply on top of branch "enterprise-4.13":
In response to this:
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. |
@aireilly: #87375 failed to apply on top of branch "enterprise-4.14":
In response to this:
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. |
@aireilly: #87375 failed to apply on top of branch "enterprise-4.15":
In response to this:
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. |
@aireilly: #87375 failed to apply on top of branch "enterprise-4.16":
In response to this:
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. |
@aireilly: #87375 failed to apply on top of branch "enterprise-4.17":
In response to this:
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. |
@aireilly: #87375 failed to apply on top of branch "enterprise-4.18":
In response to this:
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. |
For https://issues.redhat.com/browse/OSDOCS-13186
Version(s):
4.12, 4.13, 4.14, 4.15, 4.16, 4.17, 4.18
Issue:
OSDOCS 13186
Link to docs preview:
N/A
QE review:
N/A
Additional information:
This is a modification to the build_for_portal.py script, not to the documentation