-
Notifications
You must be signed in to change notification settings - Fork 670
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
Implement GetProject endpoint in FlyteAdmin #4825
Conversation
Signed-off-by: Rafael Raposo <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4825 +/- ##
==========================================
+ Coverage 58.97% 58.98% +0.01%
==========================================
Files 645 645
Lines 55578 55622 +44
==========================================
+ Hits 32778 32811 +33
- Misses 20207 20217 +10
- Partials 2593 2594 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Rafael Raposo <[email protected]>
Better code coverage
I've added the missing coverage for `project.go. |
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.
should we add rpc GetProject
to the proto?
flyte/flyteidl/protos/flyteidl/service/admin.proto
Lines 493 to 517 in bce5d8c
rpc UpdateProject (flyteidl.admin.Project) returns (flyteidl.admin.ProjectUpdateResponse) { | |
option (google.api.http) = { | |
put: "/api/v1/projects/{id}" | |
body: "*" | |
additional_bindings { | |
put: "/api/v1/projects/org/{org}/{id}" | |
body: "*" | |
} | |
}; | |
// option (grpc.gateway.protoc_gen_swagger.options.openapiv2_operation) = { | |
// description: "Update a project." | |
// }; | |
} | |
// Fetches a list of :ref:`ref_flyteidl.admin.Project` | |
rpc ListProjects (flyteidl.admin.ProjectListRequest) returns (flyteidl.admin.Projects) { | |
option (google.api.http) = { | |
get: "/api/v1/projects" | |
additional_bindings { | |
get: "/api/v1/projects/org/{org}" | |
} | |
}; | |
// option (grpc.gateway.protoc_gen_swagger.options.openapiv2_operation) = { | |
// description: "Fetch registered projects." | |
// }; |
Signed-off-by: Rafael Raposo <[email protected]>
@pingsutw added, ptal. |
Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
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.
LGTM, @katrogan, mind taking a look as well?
@RRap0so Any update on this PR? |
Signed-off-by: Rafael Raposo <[email protected]>
@pingsutw I've done the changes and I've merged with master. |
Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
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.
hey @RRap0so seeing a couple of resolved comments that weren't addressed, is this PR missing a push to remote?
Signed-off-by: Rafael Raposo <[email protected]>
@katrogan sorry about that, I've addressed all the comments. |
Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
@RRap0so no problem, just wanted to make sure all changes had been pushed! |
Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
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.
thank you for making this change!
Congrats on merging your first pull request! 🎉 |
) (#178) ## Overview Cherry-pick of [3fe4533](3fe4533) and [c21ded6](c21ded6) as well as code changes to be org-aware ## Test Plan *TODO: Summarize tests added, integration tests run, or other steps you took to validate this change. Include (or link to) relevant test output or screenshots.* ## Rollout Plan (if applicable) *TODO: Describe any deployment or compatibility considerations for rolling out this change.* ## Upstream Changes Should this change be upstreamed to OSS (flyteorg/flyte)? If so, please check this box for auditing. Note, this is the responsibility of each developer. See [this guide](https://unionai.atlassian.net/wiki/spaces/ENG/pages/447610883/Flyte+-+Union+Cloud+Development+Runbook/#When-are-versions-updated%3F). - [ ] To be upstreamed ## Jira Issue https://unionai.atlassian.net/browse/<project-number> ## Checklist * [ ] Added tests * [ ] Ran a deploy dry run and shared the terraform plan * [ ] Added logging and metrics * [ ] Updated [dashboards](https://unionai.grafana.net/dashboards) and [alerts](https://unionai.grafana.net/alerting/list) * [ ] Updated documentation
Tracking issue
Closes #4317
Why are the changes needed?
We have several services and operators that fetch single Project objects from flyteadmin. This new endpoint will save some filtering by only fetching the one we need instead of fetching a List of Projects (the endpoint available today)
Note: This replaces #4316 (diverged too much from main)
What changes were proposed in this pull request?
How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link