Migrate project admin endpoints from platform to nexus#73
Migrate project admin endpoints from platform to nexus#73jeroenrinzema merged 11 commits intomainfrom
Conversation
|
@copilot Could you please move the project admins endpoints into the admins controller? It makes more sense to group them that way. |
Done. I've moved the project admin endpoints into the admins controller and consolidated the tests into admins_test.go. The separate project_admins.go and project_admins_test.go files have been removed. All tests pass. (commit d333728) |
There was a problem hiding this comment.
Pull request overview
This PR migrates project admin management endpoints from the legacy platform service to the nexus service, implementing CRUD operations for managing admins at the project level. The migration follows nexus's OpenAPI-first architecture pattern, consolidating project admin functionality into the existing AdminsController rather than creating a separate controller.
Key Changes
- Added project admin endpoints to OpenAPI spec with list, create, get, update, and delete operations
- Implemented project admin store methods in the AdminsController for database operations
- Created comprehensive test coverage for all project admin endpoints using testcontainers
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| services/nexus/oapi/resources.yml | Defines OpenAPI spec for project admin endpoints with schemas and parameters |
| services/nexus/oapi/resources_gen.go | Generated code from OpenAPI spec including types, client methods, and server interfaces |
| services/nexus/internal/store/admins.go | Implements ProjectAdmin struct and CRUD methods for project admin operations |
| services/nexus/internal/http/controllers/v1/admins.go | Implements HTTP handlers for project admin endpoints with validation and error handling |
| services/nexus/internal/http/controllers/v1/admins_test.go | Comprehensive test coverage for all project admin operations |
| services/nexus/internal/store/migrations/1764531658_add_project_admins_unique_constraint.up.sql | Migration to add unique constraint preventing duplicate admin assignments |
| services/nexus/internal/store/migrations/1764531658_add_project_admins_unique_constraint.down.sql | Rollback migration for unique constraint |
|
@copilot apply changes based on the comments in this thread |
I've removed the unnecessary |
| admins := store.NewAdminsStore(db) | ||
|
|
||
| for i := 0; i < 3; i++ { | ||
| email := "admin" + string(rune('0'+i)) + "@example.com" |
There was a problem hiding this comment.
String concatenation logic is incorrect. Converting '0'+i to a rune and then to a string will not produce the expected numeric character. For example, when i=0, this produces '0' (character code 48), but when i>9, it produces non-numeric characters. Use strconv.Itoa(i) instead.
There was a problem hiding this comment.
Please make a slide of admin email addresses instead. This might create a more predictable test.
| existing, err := s.GetProjectAdmin(ctx, projectID, adminID) | ||
| if err != nil && !errors.Is(err, sql.ErrNoRows) { | ||
| return err | ||
| } | ||
|
|
||
| if existing != nil { | ||
| return s.UpdateProjectAdminRole(ctx, projectID, adminID, role) | ||
| } |
There was a problem hiding this comment.
[nitpick] The logic to check for existing admin and update if present creates an implicit upsert behavior that may not be obvious from the function name AddAdminToProject. Consider either renaming to UpsertAdminToProject or splitting into separate add and update flows with explicit error handling when an admin already exists.
|
@copilot apply changes based on the comments in this thread |
I've updated the test to use a slice of email addresses ( |
|
@copilot please check the linting and testing steps, they are failing at the moment |
I've fixed the build and test failures. The issue was that I removed the |
Co-authored-by: jeroenrinzema <3440116+jeroenrinzema@users.noreply.github.com>
Co-authored-by: jeroenrinzema <3440116+jeroenrinzema@users.noreply.github.com>
Co-authored-by: jeroenrinzema <3440116+jeroenrinzema@users.noreply.github.com>
Co-authored-by: jeroenrinzema <3440116+jeroenrinzema@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: jeroenrinzema <3440116+jeroenrinzema@users.noreply.github.com>
…AdminToProject Co-authored-by: jeroenrinzema <3440116+jeroenrinzema@users.noreply.github.com>
Co-authored-by: jeroenrinzema <3440116+jeroenrinzema@users.noreply.github.com>
cd4c0c9 to
7d2ec3e
Compare
make generateto generate types from OpenAPI specOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.