Conversation
Rohit0301
added
the
safe to test
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Jest test CoverageUI tests summary
|
🟡 Playwright Results — all passed (40 flaky)✅ 4438 passed · ❌ 0 failed · 🟡 40 flaky · ⏭️ 38 skipped
🟡 40 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
|
Failed to cherry-pick changes to the 1.12.13 branch. |
|
Failed to cherry-pick changes to the 1.13 branch. |
Code Review ✅ Approved 3 resolved / 3 findingsUpdates pipeline run permissions to use the trigger operation. All previously identified test and permission gaps have been resolved. ✅ 3 resolved✅ Bug: Global Playwright baseURL changed from 8585 to 3002
✅ Bug: useMemo missing appPermissions dependency causes stale permissions
✅ Bug: Unit tests assert old Deploy-permission semantics; will fail in CI
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
* Added necessary permissions for the run/deploy buttons * fixed the playwright test * addressed gitar comment * added ingestion trigger permissino instead of all * fix playwright * minor fix * lint fix * unit test fix
* Added necessary permissions for the run/deploy buttons * fixed the playwright test * addressed gitar comment * added ingestion trigger permissino instead of all * fix playwright * minor fix * lint fix * unit test fix
2 participants
Describe your changes:
Fixes 29468
I worked on ... because ...
Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
Unit tests
Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Greptile Summary
This PR fixes a permission gap where the "Run" button on ingestion pipelines was gated on
Operation.EditAllinstead of the more granularOperation.Trigger. Users with only Trigger permission couldn't run pipelines, and users with EditAll but no Trigger permission could run them incorrectly.PipelineActionsDropdown.tsx: Extracts atriggerPermissionfromOperation.Trigger(defaulting tofalse) and uses it to gate the run button, keeping deploy/re-deploy/edit/kill gated oneditPermission(EditAll).Permissions.mock.ts+PipelineActionsDropdown.test.tsx: AddsTrigger: trueto the shared mock so existing tests remain green, and adds a new unit test verifying the run button disappears whenTriggerisfalsewhileEditAllistrue.ServiceCreationPermissions.spec.ts: Adds two E2E scenarios — a Trigger-only user who can run but not edit, and an EditAll-only user who can edit/redeploy but not run.Confidence Score: 5/5
Safe to merge — the change is a small, targeted permission-gate fix on a single button with matching unit and E2E test coverage.
The component change is minimal and correct:
triggerPermissionis derived from the right operation, defaults tofalse(fail-closed), and the useMemo dependency array is updated consistently. Existing tests are preserved through theTrigger: trueaddition to the shared mock.The Playwright spec's
beforeAllsetup silently discards the deploy response; worth asserting its status to make future flakiness easier to diagnose.Important Files Changed
triggerPermissionfromOperation.Triggerand gates the run button on it instead ofeditPermission; deploy/re-deploy remain gated oneditPermission(EditAll).Operation.Triggeris false while EditAll remains true; importsENTITY_PERMISSIONSfor structured permission overrides.Trigger: truetoENTITY_PERMISSIONSso existing tests that rely on the full permission set remain passing after the component change.run-buttonvisibility assertion to fail if thedeployedflag is not set.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[PipelineActionsDropdown renders] --> B{ingestionPipelinePermissions} B --> C[editPermission = Operation.EditAll] B --> D[deletePermission = Operation.Delete] B --> E[triggerPermission = Operation.Trigger] C --> F{ingestion.deployed?} E --> F F -->|true| G{ingestion.enabled?} F -->|false| H[deploy button - hidden: !editPermission] G -->|true| I[run button - hidden: !triggerPermission] G -->|true| J[re-deploy button - hidden: !editPermission] G -->|false| K[deploy/run buttons not shown] C --> L[edit button - hidden: !editPermission] C --> M[kill button - hidden: !editPermission] D --> N[delete button - hidden: !deletePermission]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[PipelineActionsDropdown renders] --> B{ingestionPipelinePermissions} B --> C[editPermission = Operation.EditAll] B --> D[deletePermission = Operation.Delete] B --> E[triggerPermission = Operation.Trigger] C --> F{ingestion.deployed?} E --> F F -->|true| G{ingestion.enabled?} F -->|false| H[deploy button - hidden: !editPermission] G -->|true| I[run button - hidden: !triggerPermission] G -->|true| J[re-deploy button - hidden: !editPermission] G -->|false| K[deploy/run buttons not shown] C --> L[edit button - hidden: !editPermission] C --> M[kill button - hidden: !editPermission] D --> N[delete button - hidden: !deletePermission]Reviews (7): Last reviewed commit: "Merge branch 'main' into fix-29468" | Re-trigger Greptile