Skip to content

Fix unbound variable from get_ocp_docs.sh #86

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.

Sign up for GitHub

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

Open
umago wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix unbound variable from get_ocp_docs.sh #86

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 5 additions & 11 deletions scripts/get_ocp_docs.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@
set -eou pipefail
set -x

SCRIPT_DIR="$(realpath "$( dirname -- "${BASH_SOURCE[0]}" )")"
OLS_DOC_REPO=${OLS_DOC_REPO:-"https://github.com/openshift/lightspeed-rag-content.git"}
# OpenShift Versions to create DBs for
# If we set it to empty string, it will generate all the available ones.
OCP_VERSIONS=${OCP_VERSIONS:-"4.16 4.18 latest"}
Copy link
Contributor

@Akrog Akrog Jan 28, 2026

Choose a reason for hiding this comment

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

Existing code is tailored for our current make target.
Your change breaks it because we always pass it:

rag-content/Makefile

Line 20 in 02412af

OCP_VERSIONS ?= ""

I advocate for removing the u on the set command.

Copy link
Contributor

@lpiwowar lpiwowar Jan 29, 2026

Choose a reason for hiding this comment

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

@Akrog how does the change break the compatibility with the Makefile exactly? Maybe my brain is not thinking enough today but I really do not see it.

IMO this changes keeps the existing behavior intact plus ensures that the script can be used nicely standalone even without the Makefile (by dealing with the unbound variable). I like it.

I would keep the -u. I like the strictness of it. But I do not have super strong opinion on this.

Copy link
Contributor Author

@umago umago Jan 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

@Akrog thanks for the review.

@lpiwowar I think he means as the comment in the code says, if the variable is empty it will generate the docs for all available versions. But with my change, if the variable is empty it will then fallback to the default values.

I think the behavior is strange but yeah my change does break that assumption indeed. I will keep this open so we can discuss what would be best.

For context, the reason I made this change is because I was working on the per-rendering of the docs and I used the scripts in standalone mode, then I noticed this problem.

Copy link
Contributor Author

@umago umago Jan 29, 2026

Choose a reason for hiding this comment

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

I was looking at the code and if the variable is empty it will fallback to the default values as well... So I don't think this break anything ?

The comment in the code is not accurate, because in other to build all available versions we need to set OCP_VERSIONS="all"

rag-content/scripts/get_ocp_docs.sh

Lines 49 to 52 in 7858586

# If "all" versions, we need to list them ourselves
if [ "$OCP_VERSIONS" == "all" ]; then
OCP_VERSIONS="$(find lightspeed-rag-content/ocp-product-docs-plaintext/* -maxdepth 0 -printf '%f ')"
fi


if [[ -z "${OLS_DOC_REPO}" ]]; then
OLS_DOC_REPO="https://github.com/openshift/lightspeed-rag-content.git"
fi
SCRIPT_DIR="$(realpath "$( dirname -- "${BASH_SOURCE[0]}" )")"

# The current directory where the script was invoked
CURR_DIR=$(pwd)
Expand All @@ -29,13 +30,6 @@ if [[ $OUTPUT_DIR_NAME != /* ]]; then
OUTPUT_DIR_NAME="${CURR_DIR}/${OUTPUT_DIR_NAME}"
fi

# OpenShift Versions to create DBs for
# If we set it to empty string, it will generate all the available ones.
_OCP_VERSIONS="4.16 4.18 latest"
if [[ -z "${OCP_VERSIONS}" ]]; then
OCP_VERSIONS=${_OCP_VERSIONS}
fi

# Working directory
WORKING_DIR="${WORKING_DIR:-/tmp/ocp_docs_temp}"

Expand Down