refactor: formatting webserver scripts - #4086
Conversation
…sys/pymapdl into ci/adding-bash-checker
|
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
Reviewer's GuideRefactors shell scripts across CI workflows, devcontainer setups, and documentation to enforce consistent shebangs, descriptive headers, error handling patterns, standardized variable usage, and adds shellcheck linting; also introduces a unified documentation webserver script and updates the changelog. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @germa89 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `.ci/display_logs_remote.sh:12` </location>
<code_context>
# Displaying MAPDL files
-FILE_PAT=./"$LOG_NAMES"/*.err
-if compgen -G "$FILE_PAT" > /dev/null ;then for f in "$FILE_PAT"; do echo "::group:: Error file $f" && cat "$f" && echo "::endgroup::" ; done; fi || echo "Failed to display the 'out' files."
+FILE_PAT="./$LOG_NAMES/*.err"
+if compgen -G "$FILE_PAT" > /dev/null ;then for f in $FILE_PAT; do echo "::group:: Error file $f" && cat "$f" && echo "::endgroup::" ; done; fi || echo "Failed to display the 'out' files."
-FILE_PAT=./"$LOG_NAMES"/*.log
</code_context>
<issue_to_address>
Expanding glob patterns without quotes may break on filenames with spaces.
Using unquoted glob patterns can cause issues with filenames containing spaces or special characters. Use 'for f in "$FILE_PAT"' or a safer approach like 'find ... | while read f; do ...' to handle such cases correctly.
</issue_to_address>
### Comment 2
<location> `.ci/start_mapdl.sh:21` </location>
<code_context>
echo "It is latest-ubuntu. Using 'ansys' script to launch"
- export EXEC_PATH=ansys
- # export P_SCHEMA=/ansys_inc/ansys/ac4/schema
+ EXEC_PATH=ansys
+ # P_SCHEMA=/ansys_inc/ansys/ac4/schema
elif [[ $MAPDL_VERSION == *"ubuntu"* ]] ; then
</code_context>
<issue_to_address>
P_SCHEMA is not exported in the 'latest-ubuntu' branch, unlike other branches.
Downstream scripts may fail if they rely on P_SCHEMA being set, causing inconsistent behavior across branches.
</issue_to_address>
### Comment 3
<location> `doc/webserver.sh:49` </location>
<code_context>
+(cd _build/html &> /dev/null ) || { echo -e "${ERROR} HTML docs files are not found!${END_FT}"; exit 1; }
+which python &> /dev/null || { echo -e "${ERROR} Python is not available!${END_FT}" && exit 1; }
+
+python -m http.server "${PORT}" >> "${LOG_FILE}" 2>&1 & echo -n "$!" > webserver.pid
+
+echo "The pid of the web server is $(cat webserver.pid)"
</code_context>
<issue_to_address>
Writing the PID to webserver.pid may not work as intended due to backgrounding.
Use parentheses to group the background process and PID write to ensure the correct PID is captured: (python ... & echo $! > webserver.pid).
</issue_to_address>
### Comment 4
<location> `doc/webserver.sh:36` </location>
<code_context>
+PORT="${2:-${PORT:-8000}}"
+
+if [[ "$STOP" == "stop" ]]; then
+ pkill -9 -f 'm http.server' && echo -e "${BLUE}${BOLD}All web servers have been stopped.${END_FT}" || echo -e "${ERROR} Web server could not be stopped or does not exist.${END_FT}"
+ exit 0
+fi
</code_context>
<issue_to_address>
Using 'pkill -f' with a short pattern may kill unintended processes.
Consider using a more specific pattern or tracking the server's PID to avoid terminating unrelated processes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull Request Overview
Refactors documentation webserver scripts into a single script, standardizes shell scripting across devcontainers and CI, and adds ShellCheck to pre-commit.
- Introduce unified
webserver.shfor starting/stopping docs server; remove legacy start/stop scripts - Add ShellCheck hook and fix shebangs or disable warnings in devcontainer startup scripts
- Standardize error handling, glob patterns, and cd safety in CI helper scripts
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| doc/webserver.sh | New unified start/stop server script |
| doc/stop_webserver.sh | Removed legacy stop script |
| doc/start_webserver.sh | Removed legacy start script |
| .pre-commit-config.yaml | Added ShellCheck pre-commit hook |
| .devcontainer/*/start.sh | Fixed shebangs and added ShellCheck disables |
| .ci/*.sh | Standardized cd error handling, glob patterns, and variable usage |
Comments suppressed due to low confidence (4)
doc/webserver.sh:59
- Reference to 'stop_webserver.sh' is outdated since this script replaces separate start/stop scripts. Update the instruction to use './webserver.sh stop'.
echo -e "${INFO} Remember to stop the server when you are done using 'stop_webserver.sh'.${END_FT}"
.ci/display_logs_locals.sh:10
- Script header references MAPDL_LOGS_DIR and OUTPUT_DIR, but the script uses LOG_NAMES. Update the documentation to match actual variables used.
#
.ci/collect_mapdl_logs_locals.sh:10
- Header doc variables (MAPDL_LOGS_DIR, OUTPUT_DIR) don't match the code using LOG_NAMES. Align header with script behavior or use the documented variables.
# MAPDL_LOGS_DIR - Directory containing MAPDL log files to collect.
.ci/build_matrix.sh:171
- [nitpick] Consider quoting $GITHUB_OUTPUT in the redirection to handle paths with spaces:
>> "$GITHUB_OUTPUT".
echo "matrix=${JSON}" >> "$GITHUB_OUTPUT"
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4086 +/- ##
==========================================
- Coverage 91.85% 91.81% -0.05%
==========================================
Files 187 187
Lines 15033 15033
==========================================
- Hits 13809 13802 -7
- Misses 1224 1231 +7 🚀 New features to boost your workflow:
|
…g-webserver-scripts
|
@pyansys-ci-bot LGTM. |

Description
As the title.
Although they are not used much.
Issue linked
NA
Checklist
draftif it is not ready to be reviewed yet.feat: adding new MAPDL command)Summary by Sourcery
Standardize formatting, shebang usage, and environment handling across CI, devcontainer, and documentation shell scripts; add a dedicated webserver script for serving HTML docs; and integrate ShellCheck into pre-commit.
New Features:
Enhancements:
CI:
Documentation:
Chores: