refactor: encapsulate project creation logic into the ProjectCreator interface#6557
Conversation
…interface Signed-off-by: Prashantkumar Khatri <khatri2105104@st.jmi.ac.in>
intelligent-ears
left a comment
There was a problem hiding this comment.
Nice work on this, @ShantKhatri — this is a clean shape for the refactor and matches what the issue asked for. A couple of things I'd like addressed before merging, the authz one being the one I'd consider blocking.
…ject creator Signed-off-by: Prashantkumar Khatri <khatri2105104@st.jmi.ac.in>
a34c311 to
11bed21
Compare
intelligent-ears
left a comment
There was a problem hiding this comment.
The existing tests were updated for the new constructor signature (mockStore, &flags.FakeClient{}), but I don't see new tests targeting ProvisionProject itself. Since this method now owns permission checks, the feature-flag gate, and transaction boundaries — logic that previously lived in the handler — could we get coverage for:
- child-project creation when
authzClient.Checkdenies - child-project creation when
ProjectAllowsProjectHierarchyOperationsis false - top-level creation when
flags.ProjectCreateDeleteis disabled - top-level creation with no identity in context
- the
project == nilpost-creation guard
Even a few table-driven cases covering these branches would close the gap left by the handler logic moving here.
evankanderson
left a comment
There was a problem hiding this comment.
I realize that we didn't have existing coverage for this code, but it might be good to add test coverage while you're here. Other than that, a couple comments on the service interface between the handlers in controlplane and the rest of the system -- this is somewhere that we were pretty sloppy at first, but we've been trying to clean things up over time.
Fair, will add table-driven tests covering as Check denied, hierarchy flag off, top-level flag disabled, missing identity, and the nil project guard. Will be straightforward since the logic is now centralized. |
Thanks for the review. For sure, I'll be adding the test coverage as described by @intelligent-ears. |
…z checks in project handlers Signed-off-by: Prashantkumar Khatri <khatri2105104@st.jmi.ac.in>
Done! I've added a comprehensive table-driven test ( |
evankanderson
left a comment
There was a problem hiding this comment.
test coverage looks good, thanks!
Need to fix the lint error, and then should be good to merge
Co-authored-by: Evan Anderson <evan.k.anderson@gmail.com>
Signed-off-by: Prashantkumar Khatri <khatri2105104@st.jmi.ac.in>
Summary
The PR centralizes project creation logic into
internal/projects, removing duplication between the control plane and the projects module.Before, CreateProject in
handlers_projects.godirectly managed the DB transaction, authz check, and feature flag validation before delegating toProvisionChildProject / ProvisionSelfEnrolledProject.Now, a new
ProvisionProjectmethod on the ProjectCreator interface owns the full creation orchestration. The handler is now a thin gRPC adapter.Changes as,
internal/projects/creator.gois the core change.ProvisionProjectis added to theProjectCreatorinterface withdb.Storeandflags.Interfaceinjected as dependencies, so the module now owns its own transaction boundaries, authz checks, and feature flag gates. As a result,CreateProjectininternal/controlplane/handlers_projects.gois reduced to a single delegate call with no business logic.internal/service/service.gois updated to pass store and featureFlagClient to NewProjectCreator to satisfy the new dependencies. Test call sites increator_test.goandhandlers_user_test.goare updated accordingly to supply the two new constructor arguments.The flaggable intentional design made here is to
makeProjectFactoryinservice.gostill callsProvisionSelfEnrolledProjectdirectly with an existingdb.ExtendQuerier. GitHub App webhook flows arrive with an in-flight transaction that must not be re-wrapped; routing those through ProvisionProject would start a nested transaction.Fixes #5002
Testing
Install required tools
make bootstrapGenerate example rules/profiles used by some tests
make init-examplesmake lintgo test ./internal/projects/... ./internal/controlplane/... ./internal/service/...go test ./...