fix: adding new script to run the jenkin job#313
fix: adding new script to run the jenkin job#313ktyagiapphelix2u wants to merge 3 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Jenkins utility shell script for creating job-scoped Python virtual environments using Python’s built-in venv module (intended to improve compatibility with Python 3.12+).
Changes:
- Introduces
util/jenkins/virtualenv_python_tools.shwithcreate_virtualenv_pythonto create venvs under$JOBVENVDIR. - Adds basic argument parsing for
--python=...and--clear. - Automatically upgrades
pip/setuptools/wheelafter venv creation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| venvpath="$JOBVENVDIR/$venvname" | ||
|
|
||
| # create the virtualenv using python's venv module | ||
| "$python_bin" -m venv $clear_flag "$venvpath" |
There was a problem hiding this comment.
If "$python_bin" -m venv ... fails, the script still attempts to run "$venvpath/bin/python" -m pip ..., which can mask the real failure with a secondary "file not found" error. Add an early return/check after the venv creation command so the function exits immediately on failure.
| "$python_bin" -m venv $clear_flag "$venvpath" | |
| if ! "$python_bin" -m venv $clear_flag "$venvpath"; then | |
| echo "Failed to create virtualenv at '$venvpath' using '$python_bin'" >&2 | |
| return 1 | |
| fi |
| venvpath="$JOBVENVDIR/$venvname" | ||
|
|
||
| # create the virtualenv using python's venv module | ||
| "$python_bin" -m venv $clear_flag "$venvpath" |
There was a problem hiding this comment.
$clear_flag is expanded unquoted in the venv creation command. Even though it is currently controlled, using an array (or quoting/conditional inclusion) avoids word-splitting/globbing issues and matches common shell-scripting safety practices.
| "$python_bin" -m venv $clear_flag "$venvpath" | |
| if [ -n "$clear_flag" ]; then | |
| "$python_bin" -m venv "$clear_flag" "$venvpath" | |
| else | |
| "$python_bin" -m venv "$venvpath" | |
| fi |
| # Upgrade pip to avoid distutils issues with Python 3.12+ | ||
| "$venvpath/bin/python" -m pip install --upgrade pip setuptools wheel |
There was a problem hiding this comment.
The unconditional pip install --upgrade pip setuptools wheel will run on every invocation (even when reusing an existing venv), adding repeated network traffic and job time. Consider only running the upgrade when the venv is newly created/cleared, or gate it behind an environment flag so callers can opt out in performance-sensitive jobs.
| function create_virtualenv_python () { | ||
| if [ -z "${JOBVENVDIR:-}" ] | ||
| then | ||
| echo "No JOBVENVDIR found. Using default value." >&2 | ||
| JOBVENVDIR="/edx/var/jenkins/jobvenvs" | ||
| fi |
There was a problem hiding this comment.
Indentation inside create_virtualenv_python is inconsistent with the established style in util/jenkins/virtualenv_tools.sh (e.g., create_virtualenv indents the whole function body). Re-indent the body of this function to match the existing convention for readability and consistency.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/syntax-test.yml
Outdated
| max-parallel: 4 | ||
| matrix: | ||
| python-version: [3.9] | ||
| python-version: ['3.10'] |
There was a problem hiding this comment.
PR title suggests this change is only about adding a Jenkins script, but this PR also changes GitHub Actions CI (Python version and requirements file). Consider updating the PR title/description to reflect the CI changes, or splitting CI adjustments into a separate PR for clearer review/rollback.
| # create a unique hash for the job based location of where job is run | ||
| # and the python binary used | ||
| venvname="$( (echo "$python_bin"; pwd) | md5sum | cut -d' ' -f1 )" | ||
| venvpath="$JOBVENVDIR/$venvname" |
There was a problem hiding this comment.
venvname is assigned without local, which leaks it into the caller’s environment when this script is sourced. In util/jenkins/virtualenv_tools.sh, only venvpath is intentionally global; consider making venvname local here as well to avoid polluting the environment and accidental name collisions.
.github/workflows/syntax-test.yml
Outdated
| @@ -24,7 +24,7 @@ jobs: | |||
| - name: Install Dependencies | |||
| run: | | |||
| pip install demjson3 | |||
| pip install -r requirements.txt | |||
| pip install -r requirements3_12.txt | |||
There was a problem hiding this comment.
The workflow installs requirements3_12.txt (generated with Python 3.12 per its header) but runs under Python 3.10. This mismatch can lead to install failures or subtly different dependency resolution; align the interpreter with the lockfile (use Python 3.12) or install the requirements file intended for 3.10.
.github/workflows/playbook-test.yml
Outdated
| @@ -24,7 +24,7 @@ jobs: | |||
| - name: Install Dependencies | |||
| run: | | |||
| pip install demjson3 | |||
| pip install -r requirements.txt | |||
| pip install -r requirements3_12.txt | |||
There was a problem hiding this comment.
The workflow installs requirements3_12.txt (generated with Python 3.12 per its header) but runs under Python 3.10. This mismatch can lead to install failures or subtly different dependency resolution; align the interpreter with the lockfile (use Python 3.12) or install the requirements file intended for 3.10.
| "$python_bin" -m venv $clear_flag "$venvpath" | ||
|
|
||
| # Upgrade pip to avoid distutils issues with Python 3.12+ | ||
| "$venvpath/bin/python" -m pip install --upgrade pip setuptools wheel |
There was a problem hiding this comment.
This line unconditionally runs pip install --upgrade pip setuptools wheel in the Jenkins job environment, pulling the latest versions from the public index without pinning or integrity verification. If PyPI or any of these packages are compromised (or a malicious mirror/registry is configured), an attacker could execute arbitrary code in the CI environment and access build secrets or modify artifacts. To reduce this supply chain risk, install from a pinned/hashed requirements or constraints file (or an internal, trusted mirror) instead of unpinned --upgrade from the public index.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (6)
playbooks/roles/testcourses/tasks/main.yml:36
- Previously this used
include: deploy.yml tags=deploy, which applies thedeploytag to all tasks inside deploy.yml.include_tasksdoes not accept thetags=include-parameter; if you still need included tasks to be selectable via--tags deploy, useapply: { tags: [deploy] }(or switch toimport_tasksif a static include is acceptable).
- include_tasks: deploy.yml
tags:
- deploy
playbooks/roles/hotg/tasks/main.yml:144
- Previously this was
include: deploy.yml tags=deploy, which tags the tasks inside deploy.yml. Withinclude_tasks, ensure the included tasks still receive thedeploytag (e.g., viaapply: { tags: [deploy] }, or useimport_tasksif appropriate) so tag-scoped runs behave the same.
- include_tasks: deploy.yml
tags:
- deploy
playbooks/create_rds.yml:88
- Indentation here places
include_tasksat the play level rather than under thetasks:list. As written, this makes the playbook YAML structure invalid (or at least not part of the play’s tasks). It should be indented to the same level as the other tasks undertasks:.
- include_tasks: create_db_and_users.yml
when: database_connection.login_host is defined
playbooks/roles/gitreload/tasks/main.yml:87
- Previously this was
include: deploy.yml tags=deploy, which tags tasks inside deploy.yml. Withinclude_tasks, preserve that behavior usingapplytags (orimport_tasks) so--tags deployruns still execute the included tasks.
- include_tasks: deploy.yml
tags:
- deploy
playbooks/roles/demo/tasks/main.yml:43
- Previously this was
include: deploy.yml tags=deploy, which tags tasks inside deploy.yml. Withinclude_tasks, useapply: { tags: [deploy] }(orimport_tasks) if you need tag-filtered runs to behave identically.
- include_tasks: deploy.yml
tags:
- deploy
playbooks/roles/edxapp/tasks/deploy.yml:521
- This include used to be
include: tag_ec2.yml tags=deploybut now the include task is taggedremove/awsand no longer applies thedeploytag to the tasks inside tag_ec2.yml. Since tag_ec2.yml doesn’t define its own tags, this changes behavior for--tags deployruns (and may cause it to run only underremove). Consider restoringdeploytagging viaapply: { tags: [deploy] }and/or adjusting the include task’stags:to match the previous intent.
- include_tasks: tag_ec2.yml
when: COMMON_TAG_EC2_INSTANCE
tags:
- remove
- aws
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Usage: | ||
| # . /edx/var/jenkins/jobvenvs/virtualenv_python_tools.sh | ||
| # create_virtualenv_python --python=python3.12 --clear | ||
| # . "$venvpath/bin/activate" |
There was a problem hiding this comment.
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.
.github/workflows/syntax-test.yml
Outdated
| - name: Install Dependencies | ||
| run: | | ||
| pip install demjson3 | ||
| pip install -r requirements.txt | ||
| pip install -r requirements3_12.txt |
There was a problem hiding this comment.
Installing requirements3_12.txt in this job while using Python 3.10 can fail if any dependencies are 3.12-only. Consider switching to Python 3.12 for this workflow or using a requirements lockfile compiled for the selected Python.
.github/workflows/playbook-test.yml
Outdated
| max-parallel: 4 | ||
| matrix: | ||
| python-version: [3.9] | ||
| python-version: ['3.10'] |
There was a problem hiding this comment.
This workflow installs requirements3_12.txt but the matrix Python version is 3.10. Please align the Python version with the lockfile (e.g., use 3.12) or use a requirements file compiled for 3.10.
| python-version: ['3.10'] | |
| python-version: ['3.12'] |
.github/workflows/playbook-test.yml
Outdated
| - name: Install Dependencies | ||
| run: | | ||
| pip install demjson3 | ||
| pip install -r requirements.txt | ||
| pip install -r requirements3_12.txt |
There was a problem hiding this comment.
Installing requirements3_12.txt under Python 3.10 can introduce dependency incompatibilities. Either bump this workflow to Python 3.12 or switch to an appropriate requirements file for the selected interpreter.
ktyagiapphelix2u
force-pushed
the
ktyagi/archiver
branch
from
640365b to
41a3a8c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # create a unique hash for the job based location of where job is run | ||
| # and the python binary used | ||
| venvname="$( (echo "$python_bin"; pwd) | md5sum | cut -d' ' -f1 )" |
There was a problem hiding this comment.
venvname is derived only from python_bin and pwd, whereas the existing util/jenkins/virtualenv_tools.sh hashes over all creation args (virtualenv_tools.sh:40). If you later add any venv options (e.g., system-site-packages, prompt, upgrade) they could silently collide onto the same venv directory. Consider hashing over the full set of effective venv-creation inputs (e.g., python_bin + flags) to keep the venv path uniquely tied to how it was created.
| # create a unique hash for the job based location of where job is run | |
| # and the python binary used | |
| venvname="$( (echo "$python_bin"; pwd) | md5sum | cut -d' ' -f1 )" | |
| # create a unique hash for the job based location of where job is run, | |
| # the python binary used, and the venv creation flags | |
| venvname="$(printf '%s\n' "$python_bin" "$(pwd)" "$clear_flag" | md5sum | cut -d' ' -f1)" |
|
|
||
| # create a unique hash for the job based location of where job is run | ||
| # and the python binary used | ||
| venvname="$( (echo "$python_bin"; pwd) | md5sum | cut -d' ' -f1 )" |
There was a problem hiding this comment.
The restargs will need to be included here as well.
| venvname="$( (echo "$python_bin"; pwd) | md5sum | cut -d' ' -f1 )" | |
| venvname="$( (echo "$python_bin $@"; pwd) | md5sum | cut -d' ' -f1 )" |
There was a problem hiding this comment.
Implemented
| # the virtualenv's bin directory will often be too long for the OS to | ||
| # parse. | ||
|
|
||
| function create_virtualenv_python () { |
There was a problem hiding this comment.
Non-blocking suggestion: It might be better to move this function into the existing virtualenv_tools.sh, perhaps naming it create_venv. Might help with maintenance and discovery.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Up to you! In any case, this is approved.
timmc-edx
left a comment
timmc-edx
left a comment
There was a problem hiding this comment.
Looks good! I have one comment that I think needs to be addressed before merging, and another you can consider as a suggestion.
|
Failing checks aren't relevant to this PR, feel free to ignore them. I've filed https://2u-internal.atlassian.net/browse/BOMS-408 to look into it. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| JOBVENVDIR="/edx/var/jenkins/jobvenvs" | ||
| fi | ||
|
|
||
| # First argument is the python executable |
There was a problem hiding this comment.
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 |
|
|
||
| # create the virtualenv using python's venv module | ||
| # Pass any additional arguments directly to venv | ||
| "$python_bin" -m venv "$venvpath" "$@" |
There was a problem hiding this comment.
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 |
| "$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 |
There was a problem hiding this comment.
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 |
| 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 | ||
|
|
There was a problem hiding this comment.
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" |
| venvpath="$JOBVENVDIR/$venvname" | ||
|
|
||
| # create the virtualenv using python's venv module | ||
| # Pass any additional arguments directly to venv | ||
| "$python_bin" -m venv "$venvpath" "$@" |
There was a problem hiding this comment.
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.
|
|
||
| # create a unique hash for the job based location of where job is run | ||
| # and the python binary used. | ||
| venvname="$( (echo "$python_bin $@"; pwd) | md5sum | cut -d' ' -f1 )" |
There was a problem hiding this comment.
Variable expansion
| echo "No JOBVENVDIR found. Using default value." >&2 | ||
| JOBVENVDIR="/edx/var/jenkins/jobvenvs" | ||
| fi | ||
|
|
There was a problem hiding this comment.
The script doesn't verify that JOBVENVDIR exists before attempting to create the venv. If JOBVENVDIR doesn't exist, the venv command at line 50 will fail with a confusing error about being unable to create the directory. Consider adding a check to ensure JOBVENVDIR exists and is writable, or create it if it doesn't exist. This would make the error message clearer for users. Note that virtualenv_tools.sh has the same issue, but adding this check would improve the user experience.
| # 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 |
|
@ktyagiapphelix2u: As a reminder, before reaching out to Tim for a re-review, ensure all copilot PR comments in the Conversation view of this PR have been handled, starting from the top. Also, please ensure that tests are all green. assuming there isn't a reason to make an exception. Thank you. |
3 participants
Description
Adds a new Jenkins utility shell script for creating job-scoped Python virtual environments using Python’s built-in venv module (intended to improve compatibility with Python 3.12+).
Related PR
edx/jenkins-job-dsl#1827