feat(project): scaffold project manager interface#1794
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #1794 +/- ##
============================================
+ Coverage 93.91% 93.94% +0.02%
============================================
Files 118 122 +4
Lines 6423 6453 +30
============================================
+ Hits 6032 6062 +30
Misses 391 391 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
02c57e3 to
20fc2d6
Compare
tejaskash
left a comment
There was a problem hiding this comment.
I like that its available and injected through core.
tejaskash
left a comment
There was a problem hiding this comment.
LGTM, 1 clarification, not blocking
| import type { ProjectManager } from "../../handlers/project/types"; | ||
| import type { Logger } from "../../logging"; | ||
|
|
||
| interface CreateProjectManagerConfig { |
There was a problem hiding this comment.
I think this should be a type and not an interface. Interfaces are used for things that can have multiple implementations, whereas types require specific, concrete/instantiated entities. That's what we need here for the input params for this constructor, so a standard type seems more appropriate.
There was a problem hiding this comment.
I've always preferred interfaces to types since they tend to provide better error messages, and leave the door open if we want to extend them later on. The typescript language docs appear to recommend interfaces as the default here, but I do not have a strong preference here, especially since its very unlikely we'll want to extend these shapes.
I'll make the swap to types here.
There was a problem hiding this comment.
There's an important distinction between types and interfaces that I think we should respect here. Types hold data of a determinate form. They're not implemented by anything else—they are the thing. They are, in this way, concrete. Interfaces, in contrast, are abstract; they don't have a concrete existence. They must be implemented and instantiated by something else that is concrete. In the case here, what we want is a concrete input with data, which makes a type the right tool for the current job. Making this an interface would bring in a bunch of unnecessary functionality and, as you said, make it possible to extend, which is precisely what we wouldn't want in this case.
| export function createProjectManager(_config: CreateProjectManagerConfig): ProjectManager { | ||
| return { | ||
| find: (_input) => { | ||
| throw new Error(`ProjectManager.find is not implemented yet`); |
There was a problem hiding this comment.
I know we're just setting up the scaffolding here, but I tend to prefer class-based implementations of interfaces. They usually give you more flexibility going forward and, generally speaking, more developers are familiar with class-based patterns.
There was a problem hiding this comment.
I definitely agree that more developers are familiar with classes, but I personally find them overly verbose in TS. One specific example is that when implementing an interface with an object, the compiler is able to infer the types (i.e. don't need type annotations on find here), which I find to be more readable and avoid defining the same types twice.
I tend to only reach for classes if I need inheritance or instanceof checks, but I don't think either is relevant here.
There was a problem hiding this comment.
chatted with @tejaskash, sounds like I'm in the minority. swapping implementation to classes.
| CreateIamClient, | ||
| } from "./types"; | ||
|
|
||
| interface CoreClientConfig { |
There was a problem hiding this comment.
Like the above, this should be a type.
| import { PROJECT_TEMPLATES, type ProjectManager } from "../types"; | ||
|
|
||
| export const PROJECT_TEMPLATES = ["placeholder"] as const; | ||
| interface CreateProjectHandlerConfig { |
There was a problem hiding this comment.
I think this should be a type too. Or just make the ProjectManager a standard argument. I suspect we'll want to pass io as well here.
| import type { ProjectManager } from "./types"; | ||
|
|
||
| export function createProjectHandler(): Router { | ||
| interface ProjectHandlerConfig { |
There was a problem hiding this comment.
Same as above. Let's make this a type. I think we'll want io on here too as we move forward.
| export interface FindProjectInput { | ||
| /** A path to search from when locating the project root. */ | ||
| filePath: string; | ||
| } |
There was a problem hiding this comment.
These two should be types as well.
| @@ -0,0 +1,28 @@ | |||
| /** Available project templates for scaffolding new AgentCore projects. */ | |||
| export const PROJECT_TEMPLATES = ["placeholder"] as const; | |||
There was a problem hiding this comment.
I tend to think this way of typing this stuff out would be easier to work with.
export type Template = "barebones" | "strands" | "langgraph";
export const TEMPLATE_BAREBONES: Template = "barebones";There was a problem hiding this comment.
I tried to keep the same types from #1793 since I didn't see this as part of the scope of this PR.
However, I do actually like this style of deriving the type from an array/object because it gives greater flexibility by allowing the values to be enumerated at runtime. What do you think of swapping to an object so that we can have named templates too (like TEMPLATE_BAREBONES above)?
Ex.
const TEMPLATES = {
BAREBONES: 'barebones',
STRANDS: 'strands,
} as const;
type Template = (typeof TEMPLATES)[keyof typeof TEMPLATES]
There was a problem hiding this comment.
Nice! This approach looks good to me. 👍
| /** | ||
| * Exposes the ability to configure, develop, and deploy a resolved AgentCore project. | ||
| */ | ||
| export interface Project {} |
There was a problem hiding this comment.
Should project be abstract or concrete? I tend to think it should be concrete. However it might implement some or other abstract interface.
There was a problem hiding this comment.
My understanding is that we need to define an interface here to preserve the existing dependency inversion pattern. If that interface isn't Project, what would it be?
I could imagine a capability split like Deployable, Buildable, etc. but that feels overly complex for what we're doing.
I could also imagine a java-like pattern of IProject or ProjectImpl, but that doesn't feel like idiomatic typescript.
Are you suggesting we break the dependency inversion pattern here, or is there a naming/structure option I'm missing here?
There was a problem hiding this comment.
My understanding is that we need to define an interface here to preserve the existing dependency inversion pattern. If that interface isn't Project, what would it be?
I'm not sure this argument follows exactly. If you look at the interfaces defined for harness, for example, you'll see that the methods take types as input and output objects. The relevant interface is ProjectManager, not project itself.
Are you suggesting we break the dependency inversion pattern here, or is there a naming/structure option I'm missing here?
I'm suggesting that project has concrete thing, with concrete data, so it's not an abstract entity in the same way an interface is.
There was a problem hiding this comment.
Ahh I think I understand. My original plan was:
With the project manager and project abstraction, we can start filling in the gaps (ex. add resources, deploy, dev, etc.) can all be exposed through the new Project interface.
However, based on your comments, it sounds like we're looking for Project to be pure data, and not deliver functionality itself with all functionality on the project manager class.
Let me address this to align with your comment below.
2d84ef5 to
a686c50
Compare
0048ccf to
c68e6ca
Compare
5f39824 to
bfaf476
Compare
|
Updated implementation to treat project as data instead of an interface. ProjectManager will host all project functionality. |
Problem
We are currently missing a way to create and find existing projects on the local filesystem.
Solution
withProjectmiddleware for commands that requires an existing project.Note: this PR does not implement e2e functionality, but sets up the scaffolding for future contributions.
We'll likely want a working create command before filling in all of these gaps.
Verification
agentcore project create --> Error: ProjectManager.create is not implemented yet