flyteorg
[flyteadmin] add delete execution phase api
Tracking issue
Why are the changes needed?
This API provides users with a method to batch delete unnecessary execution records. It accepts the parameters project, domain, and phase, enabling the deletion of execution records that are no longer needed.
What changes were proposed in this pull request?
In the protobuf definition, define an RPC method called DeleteExecutionPhase that accepts project, domain, and phase parameters and uses the DELETE method for the deletion request. In both execution_manager and execution_repo, implement deletion logic that includes input validation and error handling. Unit tests are provided to ensure correct functionality under both successful and failure scenarios.
How was this patch tested?
"All tests passed successfully, and I have also added unit tests."
And you can also test with postman ... tools .
Labels
Please add one or more of the following labels to categorize your PR:
- added: For new features.
- changed: For changes in existing functionality.
- deprecated: For soon-to-be-removed features.
- removed: For features being removed.
- fixed: For any bug fixed.
- security: In case of vulnerabilities
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
- [ ] I updated the documentation accordingly.
- [x] All new and existing tests passed.
- [x] All commits are signed-off.
Related PRs
Docs link
Summary by Bito
This PR implements a new API endpoint for batch deleting execution phases in Flyte, introducing protobuf message types with implementations in Go, TypeScript, and Rust. It updates execution manager and repository layers, improves encapsulation by replacing direct field access with getter methods, and switches from GET to DELETE methods with proper validation and error handling.Unit tests added: True
Estimated effort to review (1-5, lower is better): 5
Code Review Agent Run #50b8fe
Actionable Suggestions - 8
-
flyteidl/gen/pb-go/flyteidl/admin/execution.pb.go - 1
- Consider adding execution phase validation · Line 1981-1981
-
flyteadmin/pkg/rpc/adminservice/execution.go - 1
- Consider using specific metric for DeleteExecutionPhase · Line 160-161
-
flyteadmin/pkg/repositories/gormimpl/execution_repo.go - 1
- Consider adding validation for execution phase · Line 175-179
-
flyteidl/clients/go/admin/mocks/AdminServiceClient.go - 1
- Consider adding nil check for opts · Line 376-379
-
flyteidl/protos/flyteidl/service/admin.proto - 1
- Consider using DELETE instead of GET · Line 671-672
-
flyteadmin/pkg/manager/impl/execution_manager.go - 1
- Consider adding context to error return · Line 1965-1965
-
flyteidl/gen/pb-go/gateway/flyteidl/service/admin.pb.gw.go - 1
- Consider adding empty string validation · Line 5811-5814
-
flyteidl/gen/pb-go/flyteidl/service/admin_grpc.pb.go - 1
- Consider adding nil check for request · Line 2045-2047
Additional Suggestions - 5
-
flyteidl/gen/pb_python/flyteidl/service/admin_pb2_grpc.py - 1
- Consider adding type hints for method · Line 721-725
-
flyteidl/gen/pb-go/gateway/flyteidl/service/admin.pb.gw.go - 1
- Consider more RESTful URL pattern design · Line 8858-8859
-
flyteidl/gen/pb-go/flyteidl/service/admin_grpc.pb.go - 2
- Consider adding matching interface method definition · Line 79-79
- Consider adding input parameter validation · Line 717-724
-
flyteidl/gen/pb-js/flyteidl.d.ts - 1
- Consider making executionPhase property optional · Line 15101-15101
Review Details
-
Files reviewed - 25 · Commit Range:
48c547a..48c547a- flyteadmin/pkg/manager/impl/execution_manager.go
- flyteadmin/pkg/manager/interfaces/execution.go
- flyteadmin/pkg/repositories/gormimpl/execution_repo.go
- flyteadmin/pkg/repositories/interfaces/execution_repo.go
- flyteadmin/pkg/rpc/adminservice/execution.go
- flyteidl/clients/go/admin/mocks/AdminServiceClient.go
- flyteidl/clients/go/admin/mocks/AdminServiceServer.go
- flyteidl/gen/pb-es/flyteidl/admin/execution_pb.ts
- flyteidl/gen/pb-es/flyteidl/service/admin_connect.ts
- flyteidl/gen/pb-go/flyteidl/admin/execution.pb.go
- flyteidl/gen/pb-go/flyteidl/service/admin.pb.go
- flyteidl/gen/pb-go/flyteidl/service/admin_grpc.pb.go
- flyteidl/gen/pb-go/gateway/flyteidl/service/admin.pb.gw.go
- flyteidl/gen/pb-js/flyteidl.d.ts
- flyteidl/gen/pb-js/flyteidl.js
- flyteidl/gen/pb_python/flyteidl/admin/execution_pb2.py
- flyteidl/gen/pb_python/flyteidl/admin/execution_pb2.pyi
- flyteidl/gen/pb_python/flyteidl/service/admin_pb2.py
- flyteidl/gen/pb_python/flyteidl/service/admin_pb2_grpc.py
- flyteidl/gen/pb_rust/flyteidl.admin.rs
- flyteidl/gen/pb_rust/flyteidl.service.tonic.rs
- flyteidl/protos/flyteidl/admin/execution.proto
- flyteidl/protos/flyteidl/service/admin.proto
- go.mod
- go.sum
-
Files skipped - 2
- flyteidl/clients/go/assets/admin.swagger.json - Reason: Filter setting
- flyteidl/gen/pb-go/gateway/flyteidl/service/admin.swagger.json - Reason: Filter setting
-
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
AI Code Review powered by 
Codecov Report
:x: Patch coverage is 38.00000% with 31 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 58.47%. Comparing base (6e5aca7) to head (63aad12).
:warning: Report is 179 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #6267 +/- ##
==========================================
- Coverage 58.48% 58.47% -0.01%
==========================================
Files 937 937
Lines 71088 71138 +50
==========================================
+ Hits 41577 41601 +24
- Misses 26359 26382 +23
- Partials 3152 3155 +3
| Flag | Coverage Δ | |
|---|---|---|
| unittests-datacatalog | 59.06% <ø> (ø) |
|
| unittests-flyteadmin | 56.25% <38.00%> (-0.02%) |
:arrow_down: |
| unittests-flytecopilot | 30.99% <ø> (ø) |
|
| unittests-flytectl | 64.70% <ø> (ø) |
|
| unittests-flyteidl | 76.12% <ø> (ø) |
|
| unittests-flyteplugins | 61.00% <ø> (ø) |
|
| unittests-flytepropeller | 54.79% <ø> (ø) |
|
| unittests-flytestdlib | 64.02% <ø> (-0.02%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Changelist by Bito
This pull request implements the following key changes.
| Key Change | Files Impacted |
|---|---|
| New Feature - API Expansion and Integration |
- - - - - - - - - - - - - - - - - - - - - - - |
| Testing - Comprehensive Test Coverage |
- - - |
| Other Improvements - Dependency Cleanup |
Code Review Agent Run #26e38f
Actionable Suggestions - 6
-
flyteidl/gen/pb-es/flyteidl/admin/execution_pb.ts - 1
- Consider more descriptive field naming · Line 1624-1625
-
flyteadmin/pkg/manager/impl/execution_manager.go - 2
- Consider more thorough phase validation · Line 1961-1963
- Consider validating workflow execution name field · Line 1970-1971
-
flyteidl/gen/pb-go/gateway/flyteidl/service/admin.pb.gw.go - 1
- Consider adding phase parameter validation · Line 5866-5866
-
flyteidl/gen/pb_rust/flyteidl.admin.rs - 1
- Consider more descriptive field names · Line 1852-1855
-
flyteadmin/pkg/repositories/interfaces/common.go - 1
- Consider adding field validation checks · Line 49-52
Additional Suggestions - 1
-
flyteidl/gen/pb-js/flyteidl.d.ts - 1
- Consider more descriptive field name · Line 15087-15088
Review Details
-
Files reviewed - 16 · Commit Range:
48c547a..739b16c- flyteadmin/pkg/manager/impl/execution_manager.go
- flyteadmin/pkg/repositories/gormimpl/execution_repo.go
- flyteadmin/pkg/repositories/interfaces/common.go
- flyteadmin/pkg/repositories/interfaces/execution_repo.go
- flyteidl/gen/pb-es/flyteidl/admin/execution_pb.ts
- flyteidl/gen/pb-go/flyteidl/admin/execution.pb.go
- flyteidl/gen/pb-go/flyteidl/service/admin.pb.go
- flyteidl/gen/pb-go/gateway/flyteidl/service/admin.pb.gw.go
- flyteidl/gen/pb-js/flyteidl.d.ts
- flyteidl/gen/pb-js/flyteidl.js
- flyteidl/gen/pb_python/flyteidl/admin/execution_pb2.py
- flyteidl/gen/pb_python/flyteidl/admin/execution_pb2.pyi
- flyteidl/gen/pb_python/flyteidl/service/admin_pb2.py
- flyteidl/gen/pb_rust/flyteidl.admin.rs
- flyteidl/protos/flyteidl/admin/execution.proto
- flyteidl/protos/flyteidl/service/admin.proto
-
Files skipped - 2
- flyteidl/clients/go/assets/admin.swagger.json - Reason: Filter setting
- flyteidl/gen/pb-go/gateway/flyteidl/service/admin.swagger.json - Reason: Filter setting
-
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
AI Code Review powered by
Code Review Agent Run #66451d
Actionable Suggestions - 0
Review Details
-
Files reviewed - 4 · Commit Range:
739b16c..c452608- flyteidl/gen/pb-go/flyteidl/service/admin.pb.go
- flyteidl/gen/pb-go/gateway/flyteidl/service/admin.pb.gw.go
- flyteidl/gen/pb_python/flyteidl/service/admin_pb2.py
- flyteidl/protos/flyteidl/service/admin.proto
-
Files skipped - 2
- flyteidl/clients/go/assets/admin.swagger.json - Reason: Filter setting
- flyteidl/gen/pb-go/gateway/flyteidl/service/admin.swagger.json - Reason: Filter setting
-
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
AI Code Review powered by
Code Review Agent Run #de2c1d
Actionable Suggestions - 0
Review Details
-
Files reviewed - 6 · Commit Range:
c452608..6fccc9b- flyteadmin/pkg/manager/impl/execution_manager.go
- flyteadmin/pkg/manager/impl/execution_manager_test.go
- flyteadmin/pkg/manager/mocks/execution_interface.go
- flyteadmin/pkg/repositories/gormimpl/execution_repo.go
- flyteadmin/pkg/repositories/interfaces/common.go
- flyteadmin/pkg/repositories/mocks/execution_repo.go
-
Files skipped - 0
-
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
AI Code Review powered by
Code Review Agent Run Status
- Limitations and other issues: ❌ Failure - Bito Code Review Agent didn't review this pull request automatically because it exceeded the size limit. No action is needed if you didn't intend for the agent to review it. Otherwise, you can initiate the review by typing
/reviewin a comment below.
Code Review Agent Run Status
- Limitations and other issues: ❌ Failure - Bito Code Review Agent didn't review this pull request automatically because it exceeded the size limit. No action is needed if you didn't intend for the agent to review it. Otherwise, you can initiate the review by typing
/reviewin a comment below.
before we proceed with deleting items from the db can we maybe write up an issue to address
- what we're trying to solve (mass terminate executions)
- why the current api is insufficient
- and if the current api works, how we can use the remote client in flytekit SDK to allow us to batch terminate executions and accepts a list of input parameters (created at window, current phase filters, etc)
I think as it stands now this PR could orphan executions unintentionally if we're not careful or make it really easy to delete from the DB which is a departure from how flyteadmin currently records entities. Flyteadmin allows us to archive or tombstone various entities but delete is a scarier tool
