-
Notifications
You must be signed in to change notification settings - Fork 435
[no-relnote] Test multiple driver branches during E2E #1201
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
[no-relnote] Test multiple driver branches during E2E #1201
Conversation
40b572a to
a4b8b66
Compare
Pull Request Test Coverage Report for Build 16420617111Details
💛 - Coveralls |
.github/workflows/e2e.yaml
Outdated
| aws_secret_access_key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | ||
| aws_ssh_key: ${{ secrets.AWS_SSH_KEY }} | ||
| holodeck_config: "tests/e2e/infra/aws.yaml" | ||
| holodeck_config: ${{ matrix.holodeck_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.
| holodeck_config: ${{ matrix.holodeck_file }} | |
| holodeck_config: tests/e2e/infra/aws_${{ matrix.variant }} |
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.
done
.github/workflows/e2e.yaml
Outdated
| - variant: EOL | ||
| holodeck_file: tests/e2e/infra/aws_EOL.yaml | ||
| - variant: LATEST | ||
| holodeck_file: tests/e2e/infra/aws_LATEST.yaml |
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.
We can remove this.
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.
done
.github/workflows/e2e.yaml
Outdated
| exclude: | ||
| - variant: EOL | ||
| ispr: true |
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.
I would rather run against an older driver on PRs and LATEST on main.
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.
I see, ok
dfaf0f6 to
2ba86e7
Compare
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
2ba86e7 to
7af2955
Compare
| name: ginkgo-logs-${{ matrix.driver_branch }} | ||
| path: ginkgo.json | ||
| retention-days: 15 | ||
| - name: Send Slack alert notification |
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.
Nit: Did we also want to include the driver branch in the Slack message?
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.
Good idea!
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.
done
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
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.
Pull Request Overview
This pull request enhances the end-to-end (E2E) testing workflow by introducing a matrix strategy to test multiple driver branches (550 and 575) instead of the previous single infrastructure variant approach. The changes include conditional execution logic to skip driver branch 575 for pull request events while allowing both branches to run on other events.
- Added matrix strategy to test driver branches 550 and 575 with conditional execution
- Created new driver branch-specific configuration file for branch 575
- Updated job names, configuration paths, and artifact names to reflect the driver branch being tested
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/e2e/infra/driver-branch-575.yaml | New configuration file for driver branch 575 with infrastructure specifications |
| .github/workflows/e2e.yaml | Updated workflow to use matrix strategy for multiple driver branches with conditional execution |
Comments suppressed due to low confidence (1)
.github/workflows/e2e.yaml:42
- The matrix variable name 'is_pull_request' is misleading as it actually represents the inverse - whether it's NOT a pull request. Consider renaming to 'is_not_pull_request' or 'run_all_branches' for clarity.
is_pull_request:
| strategy: | ||
| matrix: | ||
| is_pull_request: | ||
| - ${{!( startsWith(github.ref, 'refs/pull/') )}} |
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.
One question: Why the !?
This pull request enhances the end-to-end (E2E) testing workflow by introducing a matrix strategy for testing multiple infrastructure variants (EOL and LATEST) and updating the associated configuration files. The changes aim to improve flexibility and maintainability in the E2E testing process.
Workflow Enhancements:
.github/workflows/e2e.yaml: Added a matrix strategy to test both EOL and LATEST infrastructure variants, with conditional execution to skip the EOL variant for pull request events. Updated job names, Holodeck configuration, and artifact names to reflect the variant being tested. [1] [2] [3]Infrastructure Configuration Updates:
tests/e2e/infra/aws_EOL.yaml: Renamed fromaws.yamland modified to reflect the EOL variant setup by removing theimageIdfield.tests/e2e/infra/aws_LATEST.yaml: Added a new configuration file for the LATEST infrastructure variant, specifying details such as instance type, region, ingress IP ranges, and NVIDIA driver installation.