feat(project): implement remove command - #2037
Conversation
github-actions
Bot
added
the
agentcore-harness-reviewing
github-actions
Bot
removed
the
agentcore-harness-reviewing
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2037 +/- ##
=========================================
Coverage 97.13% 97.14%
=========================================
Files 381 381
Lines 22782 22824 +42
=========================================
+ Hits 22130 22172 +42
Misses 652 652 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| ); | ||
| }); | ||
|
|
||
| async function run(args: string[]) { |
There was a problem hiding this comment.
as a follow-up, I think we can pull this and inProject below out into a common location (src/testing). I have a few open PRs that develop similar utils for this.
| const agentCoreSpecPath = this.getProjectSpecPath(project); | ||
| const projectSpecKey = toProjectSpecKey(input.resourceType); | ||
|
|
||
| const existingProjectSpec = await this.json.read(agentCoreSpecPath, ProjectSpecSchema); |
There was a problem hiding this comment.
we could probably re-use some logic from add, but want to wait before trying to generalize this.
notgitika
left a comment
notgitika
left a comment
There was a problem hiding this comment.
This PR looks good thanks for setting it up. I have a suggestion I would like addressed that I left a comment below on.
separately, I have a question for the future implementations we do: will we also be deleting any code we scaffold (for runtime and harness for instance) when the user runs this command?
I spent some time reviewing the code on main and it seems mixed.
For remove gateway target, it removes the code: https://github.com/aws/agentcore-cli/blob/main/src/cli/operations/remove/remove-gateway-target.ts#L201
Same for code based evals: https://github.com/aws/agentcore-cli/blob/main/src/cli/primitives/EvaluatorPrimitive.ts#L261
however, for agent/runtime, we don't do the same: https://github.com/aws/agentcore-cli/blob/main/src/cli/primitives/AgentPrimitive.tsx#L222
Can we decide and align on one experience before we start with the rest of the implementation?
| this.logger | ||
| .child({ input }) | ||
| .warn(`unable to remove resource from project that does not exist.`); |
There was a problem hiding this comment.
can we throw right after this? at this point, the handler would still return a user message like remove successful. I'm okay to keep it this way like a no-op but it might be a bit misleading.
There was a problem hiding this comment.
I think an idempotent delete is easier, especially if customers are using this in some kind of automation.
|
good callout, I think we should remove the scaffolded code to be consistent. |
| break; | ||
| } | ||
| } catch (e) { | ||
| throw new ProjectStateError( |
There was a problem hiding this comment.
decided to throw here since it could lead to bad state. ex. i remove a runtime named 'X", then try to recreate (it would fail since the files still exist).
just had an offline discussion on why we decided not to delete the code early on. I would lean on not deleting the code, since the user could wanna reuse the code if they maybe make some changes. That is the pattern we established with with agent/runtime initially but the following code with gateway targets/code based evals were regressions, since it wasn't documented properly. what do you think? |
This reverts commit 77f7c0d.
|
responded internally, swapping back to original for now, we can add clean up later if we see a need. |
3 participants
Problem
There is no remove command!
Solution
Implement remove as a single command, taking the resource as an argument. Looking at the old CLI https://github.com/aws/agentcore-cli/blob/main/src/cli/primitives/, we basically duplicated this pattern many times, but here we instead do it generically.
Note: we don't yet support remove all, but one could imagine adding it as a resource type with special behavior.
Testing
unit tests with harness and runtime (since those are the only ones working e2e).