-
Notifications
You must be signed in to change notification settings - Fork 6
fix: adding new script to run the jenkin job #313
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,57 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| #!/usr/bin/env bash | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| # function to create a virtual environment using python's built-in venv module | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # instead of the virtualenv command. This is useful for Python 3.12+ where | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # distutils has been removed and older versions of pip/setuptools may fail. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Usage: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # . /edx/var/jenkins/jobvenvs/virtualenv_python_tools.sh | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # create_virtualenv_python python3.12 --clear | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # . "$venvpath/bin/activate" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Arguments: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # python_bin - Path to or name of the python executable (mandatory) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # additional args - Any additional arguments to pass to venv module | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Optional Environmental Variables: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # JOBVENVDIR - where on the system to create the virtualenv | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # - e.g. /edx/var/jenkins/jobvenvs | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Reason for existence: The original virtualenv_tools.sh uses the 'virtualenv' | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # command which doesn't work well with Python 3.12+ due to distutils removal. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # This script uses python's built-in venv module directly, allowing us to | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # specify exact python binary paths and avoid compatibility issues. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Why not create virtual environments right in the jenkins workspace | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # where the job is run? Because workspaces are so deep in the filesystem | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # that the autogenerated shebang line created by virtualenv on things in | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # the virtualenv's bin directory will often be too long for the OS to | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # parse. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| function create_virtualenv_python () { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking suggestion: It might be better to move this function into the existing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We thought that when the virtual env or jenkin version will get upgrade we will remove this script for this particular jenkin job so in last it will be going to remove itself. Thanks for the suggestion.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Up to you! In any case, this is approved. |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| if [ -z "${JOBVENVDIR:-}" ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| then | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "No JOBVENVDIR found. Using default value." >&2 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| JOBVENVDIR="/edx/var/jenkins/jobvenvs" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Ensure JOBVENVDIR exists and is writable before creating the venv | |
| if [ ! -d "$JOBVENVDIR" ]; then | |
| if ! mkdir -p "$JOBVENVDIR"; then | |
| echo "Error: JOBVENVDIR '$JOBVENVDIR' does not exist and could not be created." >&2 | |
| return 1 | |
| fi | |
| fi | |
| if [ ! -w "$JOBVENVDIR" ]; then | |
| echo "Error: JOBVENVDIR '$JOBVENVDIR' is not writable." >&2 | |
| return 1 | |
| fi |
Copilot
AI
Feb 6, 2026
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.
Missing validation for the mandatory python_bin argument. If the function is called without any arguments, the script will proceed with an empty python_bin, causing cryptic errors at line 50. Add a check to verify that python_bin is not empty and exit with an error message if it is.
| # First argument is the python executable | |
| # First argument is the python executable; validate it's provided | |
| if [ -z "${1:-}" ]; then | |
| echo "Error: python_bin argument is required for create_virtualenv_python." >&2 | |
| echo "Usage: create_virtualenv_python <python_bin> [venv args...]" >&2 | |
| return 1 | |
| fi |
Copilot
AI
Feb 6, 2026
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.
Variable expansion
Copilot
AI
Feb 6, 2026
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.
Missing error handling for venv creation command. If the python binary doesn't exist, doesn't support venv module, or if venv creation fails for any reason, the script will continue to line 53 and try to execute a non-existent Python binary, resulting in a confusing error. Add error checking after the venv command to ensure it succeeded before proceeding.
| "$python_bin" -m venv "$venvpath" "$@" | |
| "$python_bin" -m venv "$venvpath" "$@" | |
| venv_status=$? | |
| if [ $venv_status -ne 0 ] || [ ! -x "$venvpath/bin/python" ]; then | |
| echo "Failed to create virtualenv at '$venvpath' using '$python_bin' (exit status: $venv_status)" >&2 | |
| return 1 | |
| fi |
Copilot
AI
Feb 6, 2026
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.
Inconsistent variable ordering compared to the similar virtualenv_tools.sh. In virtualenv_tools.sh, venvpath is set after the virtualenv command (lines 43 then 47), while here venvpath is set before the venv command (lines 46 then 50). This is a deviation from the established pattern in the codebase without clear benefit and could cause issues if venv creation fails.
Copilot
AI
Feb 6, 2026
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.
Missing error handling for pip upgrade command. If pip installation fails, the script completes silently without indicating that the virtual environment may be in a broken or incomplete state. Add error checking to ensure pip upgrade succeeds, or at least warn if it fails.
| "$venvpath/bin/python" -m pip install --upgrade pip setuptools wheel | |
| if ! "$venvpath/bin/python" -m pip install --upgrade pip setuptools wheel; then | |
| echo "Warning: Failed to upgrade pip/setuptools/wheel in virtualenv at '$venvpath'. The environment may be incomplete or unstable." >&2 | |
| fi |
Copilot
AI
Feb 6, 2026
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.
The venvpath variable is set at line 46 but it's only accessible if the venv command succeeds. If venv creation fails, venvpath is still exported to the global scope pointing to a potentially non-existent or partial directory, which could lead to confusing errors in calling code. Consider setting venvpath only after successful venv creation, or explicitly unsetting it on failure.
| venvpath="$JOBVENVDIR/$venvname" | |
| # create the virtualenv using python's venv module | |
| # Pass any additional arguments directly to venv | |
| "$python_bin" -m venv "$venvpath" "$@" | |
| # Upgrade pip to avoid distutils issues with Python 3.12+ | |
| "$venvpath/bin/python" -m pip install --upgrade pip setuptools wheel | |
| local _venvpath="$JOBVENVDIR/$venvname" | |
| # create the virtualenv using python's venv module | |
| # Pass any additional arguments directly to venv | |
| if ! "$python_bin" -m venv "$_venvpath" "$@"; then | |
| unset venvpath | |
| return 1 | |
| fi | |
| # Upgrade pip to avoid distutils issues with Python 3.12+ | |
| if ! "$_venvpath/bin/python" -m pip install --upgrade pip setuptools wheel; then | |
| unset venvpath | |
| return 1 | |
| fi | |
| # Only expose venvpath globally after successful creation and setup. | |
| venvpath="$_venvpath" |
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.
This new helper script is not referenced anywhere else in the repo (no Jenkins job, playbook, or role copies/sources it). Given the PR title implies it will be used to run Jenkins jobs, please either (a) wire it into the Jenkins provisioning (e.g., copy to the job venv directory alongside virtualenv_tools.sh) or (b) update documentation/PR scope to clarify how it’s intended to be consumed.