PDP-817: Added support to execute regressions on arm infrastructure #1891
Conversation
nagalakshmi2024
requested review from
BillFarber,
rjrudin and
stevebio
as code owners
nagalakshmi2024
changed the title
|
Copyright Validation Results ⏭️ Skipped (Excluded) Files
✅ All files have valid copyright headers! |
There was a problem hiding this comment.
Pull request overview
This PR adds support for running regression tests on ARM-based infrastructure. When the arm_regressions parameter is enabled, the pipeline provisions ARM EC2 instances, configures them with necessary dependencies, and executes tests against ARM-compatible MarkLogic images.
Changes:
- Added ARM platform support with conditional logic based on
arm_regressionsparameter - Introduced new pipeline stages for provisioning ARM infrastructure and running tests
- Updated Docker Compose configuration to support both ARM and AMD64 platforms
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| docker-compose.yaml | Made platform and converter settings configurable via environment variables to support ARM |
| Jenkinsfile | Added ARM regression parameter, infrastructure provisioning stage, ARM-specific test execution, and cleanup logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export PLATFORM=$PLATFORM | ||
| export SET_CONVERTERS=$SET_CONVERTERS |
There was a problem hiding this comment.
The environment variable names are identical to the variable names being assigned ($PLATFORM and $SET_CONVERTERS). This creates ambiguity about which values are being used and makes the code harder to follow. Consider renaming the shell variables to avoid shadowing, e.g., export PLATFORM=$PLATFORM_VALUE or use more descriptive names that clarify the source.
| export PLATFORM=$PLATFORM | |
| export SET_CONVERTERS=$SET_CONVERTERS |
There was a problem hiding this comment.
Same thing here - avoid using "SET_CONVERTERS", use "MARKLOGIC_INSTALL_CONVERTERS".
| echo "Waiting for MarkLogic server to initialize." | ||
| sleep 60 |
There was a problem hiding this comment.
The 60-second sleep is a magic number without explanation of why this duration was chosen. Consider adding a comment explaining the initialization time requirement or defining this as a named constant for better maintainability.
There was a problem hiding this comment.
Remove this sleep command, it's no longer needed (remove the echo command too).
| expression { | ||
| return !params.regressions && !params.arm_regressions | ||
| } |
There was a problem hiding this comment.
The multi-line expression formatting is inconsistent with other single-line expressions in the file (e.g., lines 253-254, 274-275). For consistency, consider formatting this as a single line: expression { return !params.regressions && !params.arm_regressions }.
| expression { | |
| return !params.regressions && !params.arm_regressions | |
| } | |
| expression { return !params.regressions && !params.arm_regressions } |
| platform: "${PLATFORM:-linux/amd64}" | ||
| environment: | ||
| - INSTALL_CONVERTERS=true | ||
| - INSTALL_CONVERTERS=${SET_CONVERTERS:-true} |
There was a problem hiding this comment.
Please rename SET_CONVERTERS to MARKLOGIC_INSTALL_CONVERTERS - I don't like to introduce a new verb for something that already exists as otherwise, in the future, someone's going to wonder "What's the difference between 'installing converters' and 'setting conveters'?"
| export PLATFORM=$PLATFORM | ||
| export SET_CONVERTERS=$SET_CONVERTERS |
There was a problem hiding this comment.
Same thing here - avoid using "SET_CONVERTERS", use "MARKLOGIC_INSTALL_CONVERTERS".
| echo "Waiting for MarkLogic server to initialize." | ||
| sleep 60 |
There was a problem hiding this comment.
Remove this sleep command, it's no longer needed (remove the echo command too).
The changes made to the jenkinsfile to support
provision arm based infrastructure and execute regressions on arm ec2 instance when arm_regressions parameter is enabled
Jenkins console log for arm regressions
https://ml-clt-jenkins.progress.com/job/devexp/job/Java-Client/job/java-client-api-regression/job/arm-regressions-testbranch/108/console