-
Notifications
You must be signed in to change notification settings - Fork 164
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
[RORDEV-1402] Manual job for integration tests in Azure pipelines #1079
base: develop
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis change introduces a new pipeline stage called Sequence Diagram(s)sequenceDiagram
participant User as Manual Trigger
participant Pipeline as Azure Pipelines
participant Job1 as Integration Job (es816x-es80x)
participant Job2 as Integration Job (es717x-es67x)
User->>Pipeline: Initiate Manual Build
Pipeline->>Job1: Trigger Job in openjdk:22-slim container
Pipeline->>Job2: Trigger Job in openjdk:22-slim container
Job1->>Job1: Checkout Code
Job1->>Job1: Export AWS Credentials
Job1->>Job1: Execute Pipeline Script
Job1->>Pipeline: Publish Test Results on Failure
Job2->>Job2: Checkout Code
Job2->>Job2: Export AWS Credentials
Job2->>Job2: Execute Pipeline Script
Job2->>Pipeline: Publish Test Results on Failure
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
azure-pipelines.yml (1)
531-590
: First Integration Test Job Setup with Matrix (IT_es816x - IT_es80x)This job is set to run inside the
openjdk:22-slim
container with a 120‑minute timeout, and its steps (checkout, environment export, script execution, and publishing test results) are structured appropriately. Consider refactoring common steps into a reusable template to DRY up the configuration if similar patterns persist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
azure-pipelines.yml
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: ror (Run all tests IT_es67x)
- GitHub Check: ror (Run all tests IT_es80x)
- GitHub Check: ror (Run all tests IT_es70x)
- GitHub Check: ror (Run all tests IT_es810x)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests IT_es816x)
- GitHub Check: ror (Run all tests LICENSE)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (CVE check Job)
🔇 Additional comments (1)
azure-pipelines.yml (1)
526-530
: Stage Configuration for Manual Integration TestsThe new
INTEGRATION_TEST
stage is clearly configured to run only when the build is manually triggered. UsingdependsOn: [ ]
ensures it runs independently, which aligns with the PR objective.
timeoutInMinutes: 120 | ||
steps: | ||
- checkout: self | ||
fetchDepth: 1 | ||
clean: false | ||
persistCredentials: true | ||
- script: | | ||
# Translate back env vars to avoid cyclical reference :/ | ||
export aws_access_key_id=$var_aws_access_key_id | ||
export aws_secret_access_key=$var_aws_secret_access_key | ||
|
||
echo "[TEST] executing ROR_TASK = $ROR_TASK" | ||
ci/run-pipeline.sh | ||
env: | ||
var_aws_access_key_id: $(aws_access_key_id) | ||
var_aws_secret_access_key: $(aws_secret_access_key) | ||
- task: PublishTestResults@2 | ||
condition: failed() | ||
inputs: | ||
testRunTitle: "$(ROR_TASK) results" | ||
testResultsFiles: "**/TEST*xml" | ||
mergeTestResults: true | ||
strategy: | ||
maxParallel: 99 | ||
matrix: | ||
IT_es717x: | ||
ROR_TASK: integration_es717x | ||
IT_es716x: | ||
ROR_TASK: integration_es716x | ||
IT_es714x: | ||
ROR_TASK: integration_es714x | ||
IT_es711x: | ||
ROR_TASK: integration_es711x | ||
IT_es710x: | ||
ROR_TASK: integration_es710x | ||
IT_es79x: | ||
ROR_TASK: integration_es79x | ||
IT_es78x: | ||
ROR_TASK: integration_es78x | ||
IT_es77x: | ||
ROR_TASK: integration_es77x | ||
IT_es74x: | ||
ROR_TASK: integration_es74x | ||
IT_es73x: | ||
ROR_TASK: integration_es73x | ||
IT_es72x: | ||
ROR_TASK: integration_es72x | ||
IT_es70x: | ||
ROR_TASK: integration_es70x | ||
IT_es67x: | ||
ROR_TASK: integration_es67x | ||
|
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.
🛠️ Refactor suggestion
Second Integration Test Job: Missing Container Declaration
While the first job explicitly uses the openjdk:22-slim
container, the second job omits it. According to the integration test requirements, both jobs should run within the same container environment. To ensure consistency and avoid potential environment discrepancies, please add the container directive. For example:
- - job:
- timeoutInMinutes: 120
+ - job:
+ container: openjdk:22-slim
+ timeoutInMinutes: 120
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
timeoutInMinutes: 120 | |
steps: | |
- checkout: self | |
fetchDepth: 1 | |
clean: false | |
persistCredentials: true | |
- script: | | |
# Translate back env vars to avoid cyclical reference :/ | |
export aws_access_key_id=$var_aws_access_key_id | |
export aws_secret_access_key=$var_aws_secret_access_key | |
echo "[TEST] executing ROR_TASK = $ROR_TASK" | |
ci/run-pipeline.sh | |
env: | |
var_aws_access_key_id: $(aws_access_key_id) | |
var_aws_secret_access_key: $(aws_secret_access_key) | |
- task: PublishTestResults@2 | |
condition: failed() | |
inputs: | |
testRunTitle: "$(ROR_TASK) results" | |
testResultsFiles: "**/TEST*xml" | |
mergeTestResults: true | |
strategy: | |
maxParallel: 99 | |
matrix: | |
IT_es717x: | |
ROR_TASK: integration_es717x | |
IT_es716x: | |
ROR_TASK: integration_es716x | |
IT_es714x: | |
ROR_TASK: integration_es714x | |
IT_es711x: | |
ROR_TASK: integration_es711x | |
IT_es710x: | |
ROR_TASK: integration_es710x | |
IT_es79x: | |
ROR_TASK: integration_es79x | |
IT_es78x: | |
ROR_TASK: integration_es78x | |
IT_es77x: | |
ROR_TASK: integration_es77x | |
IT_es74x: | |
ROR_TASK: integration_es74x | |
IT_es73x: | |
ROR_TASK: integration_es73x | |
IT_es72x: | |
ROR_TASK: integration_es72x | |
IT_es70x: | |
ROR_TASK: integration_es70x | |
IT_es67x: | |
ROR_TASK: integration_es67x | |
- job: | |
container: openjdk:22-slim | |
timeoutInMinutes: 120 | |
steps: | |
- checkout: self | |
fetchDepth: 1 | |
clean: false | |
persistCredentials: true | |
- script: | | |
# Translate back env vars to avoid cyclical reference :/ | |
export aws_access_key_id=$var_aws_access_key_id | |
export aws_secret_access_key=$var_aws_secret_access_key | |
echo "[TEST] executing ROR_TASK = $ROR_TASK" | |
ci/run-pipeline.sh | |
env: | |
var_aws_access_key_id: $(aws_access_key_id) | |
var_aws_secret_access_key: $(aws_secret_access_key) | |
- task: PublishTestResults@2 | |
condition: failed() | |
inputs: | |
testRunTitle: "$(ROR_TASK) results" | |
testResultsFiles: "**/TEST*xml" | |
mergeTestResults: true | |
strategy: | |
maxParallel: 99 | |
matrix: | |
IT_es717x: | |
ROR_TASK: integration_es717x | |
IT_es716x: | |
ROR_TASK: integration_es716x | |
IT_es714x: | |
ROR_TASK: integration_es714x | |
IT_es711x: | |
ROR_TASK: integration_es711x | |
IT_es710x: | |
ROR_TASK: integration_es710x | |
IT_es79x: | |
ROR_TASK: integration_es79x | |
IT_es78x: | |
ROR_TASK: integration_es78x | |
IT_es77x: | |
ROR_TASK: integration_es77x | |
IT_es74x: | |
ROR_TASK: integration_es74x | |
IT_es73x: | |
ROR_TASK: integration_es73x | |
IT_es72x: | |
ROR_TASK: integration_es72x | |
IT_es70x: | |
ROR_TASK: integration_es70x | |
IT_es67x: | |
ROR_TASK: integration_es67x |
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
azure-pipelines.yml (1)
603-654
:⚠️ Potential issueSecond Integration Test Job: Missing Container Declaration
The second job in theINTEGRATION_TEST
stage lacks the container declaration, which is required for consistency with the first job. This discrepancy could lead to environment differences between the two jobs. Please add the following to the job configuration:+ container: openjdk:22-slim
🧹 Nitpick comments (1)
azure-pipelines.yml (1)
34-41
: Addition of Integration Tests Parameter
The new boolean parameterrunIntegrationTests
is configured correctly with a default offalse
and an explicit values list. Given that booleans naturally take onlytrue
orfalse
, consider if thevalues
block is necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
azure-pipelines.yml
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (Run all tests LICENSE)
- GitHub Check: ror (Run all tests IT_es67x)
- GitHub Check: ror (Run all tests IT_es80x)
- GitHub Check: ror (Run all tests IT_es70x)
- GitHub Check: ror (Run all tests IT_es810x)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (CVE check Job)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests IT_es816x)
🔇 Additional comments (3)
azure-pipelines.yml (3)
7-7
: New Variable Mapping for Integration Tests
The variablerunIntegrationTests
is now mapped from the parameter, which is appropriate. Ensure that this mapping is referenced consistently throughout the pipeline configuration.
534-541
: New Stage Definition for Integration Tests
TheINTEGRATION_TEST
stage is properly set up to run only for manual builds whenrunIntegrationTests
is enabled. The condition using both the build reason and the parameter is clear and meets the PR objectives.
543-602
: First Integration Test Job: Container Usage and Matrix Configuration
This first job within theINTEGRATION_TEST
stage correctly specifies theopenjdk:22-slim
container, a sensible timeout, and a comprehensive matrix covering the intended ES integration tests.
Summary by CodeRabbit