Fix unbound variable from get_ocp_docs.sh#86
Conversation
Trivial fix for the get_ocp_docs.sh script, trying to use it without setting the OLS_DOC_REPO and OCP_VERSIONS explicitly failed with an "unbound variable" error instead of fallbacking to the default values. Signed-off-by: Lucas Alvares Gomes <lucasagomes@gmail.com>
| 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"} |
There was a problem hiding this comment.
Existing code is tailored for our current make target.
Your change breaks it because we always pass it:
Line 20 in 02412af
I advocate for removing the u on the set command.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe script refactors environment variable initialization by setting defaults via parameter expansion instead of inline conditionals, and reorders variable declarations to establish directory context earlier in execution. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@umago I just wanted to check that we can trigger Code Rabbit on old PRs and this looked as a good candidate (it is a small change:). |
3 participants
Trivial fix for the get_ocp_docs.sh script, trying to use it without setting the OLS_DOC_REPO and OCP_VERSIONS explicitly failed with an "unbound variable" error instead of fallbacking to the default values.
Summary by CodeRabbit