ci(e2e): run operator e2e on main push, drop helm matrix#406
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 21 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdated the e2e GitHub Actions workflow: added a push trigger for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
some tests marked as missing, but just because we need to change the project config due to the new matrix. |
There was a problem hiding this comment.
The PR correctly:
- Adds
pushtrigger onmainbranch for e2e tests - Removes
helmfrom the matrixmethoddimension - Removes the ARM/helm
excluderule - Hardcodes
METHOD: operatorinstead of using${{ matrix.method }}
One medium-severity concern identified below regarding the interaction between the new push trigger and the existing dorny/paths-filter gate.
|
|
||
| on: | ||
| workflow_dispatch: | ||
| push: |
There was a problem hiding this comment.
[AI-generated, human reviewed]
With the new push trigger, the e2e-tests job condition at line 38 may skip tests on push-to-main:
if: needs.changes.outputs.should_run == 'true' || github.event_name == 'workflow_dispatch'For push events, github.base_ref is empty, so dorny/paths-filter falls back to the base parameter ('main'). On squash merges or force-pushes, the push payload's before SHA may not be a direct ancestor, causing the filter to produce unexpected results. Since the if condition doesn't include github.event_name == 'push', e2e tests could be silently skipped on push to main.
There was a problem hiding this comment.
oh right, I forgot we had that filter, checking, thanks! :)
There was a problem hiding this comment.
Ok, now the filtering base is fixed, and we always run the main E2E tests on all pushes (not the compat ones), as we just want an indication of any patch introducing flakiness in our codebase over the time.
|
@raballew Thanks for the detailed review. paths-filter on Job Pushed as a single amended commit on |
e1735c4 to
2130b39
Compare
- Trigger workflow on push to main so merges are covered. - Run e2e with METHOD=operator only; remove helm matrix leg and ARM/helm exclude. Made-with: Cursor
2130b39 to
b96098a
Compare
Made-with: Cursor