feat: add config-bundle CLI commands - #1945
Conversation
github-actions
Bot
added
the
agentcore-harness-reviewing
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## refactor #1945 +/- ##
============================================
+ Coverage 96.78% 96.81% +0.03%
============================================
Files 326 335 +9
Lines 18061 18388 +327
============================================
+ Hits 17480 17803 +323
- Misses 581 585 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
removed
the
agentcore-harness-reviewing
| options: CoreOptions, | ||
| ): Promise<UpdateConfigurationBundleResponse> { | ||
| const control = this.clients.control(toClientConfig(options)); | ||
| const current = await control.send(new GetConfigurationBundleCommand({ bundleId: id })); |
There was a problem hiding this comment.
I may be missing the intended branch behavior, but can we select the branch explicitly before choosing the parent version? I live-tested a mainline v1 followed by a review-branch v2. GetConfigurationBundle without branchName returned v2, and this CLI update then created v3 on review-branch. Passing branchName: "mainline" correctly returned v1. Would it make sense to expose --branch-name, default it to mainline, and pass it to both Get and Update so an ordinary update cannot silently continue whichever branch was modified most recently?
There was a problem hiding this comment.
Agreed it makes sense to have branch name here. Good catch. I've added the flag and updated tests around it
| export type CreateConfigurationBundleInput = Pick< | ||
| CreateConfigurationBundleRequest, | ||
| "bundleName" | "components" | "kmsKeyArn" | ||
| >; |
There was a problem hiding this comment.
Are these field exclusions intentional? The live service accepted description, branchName, commitMessage, createdBy, and tags during create, and accepted description plus branchName during update when the required components, commit message, and parent were supplied. The SDK also models bundleName and explicit parentVersionIds. Would it make sense to expose the common scalar fields and typed JSON for the structured ones? Also definitely get if you have reasoning behind why these are not included.
There was a problem hiding this comment.
These exclusions were intentional for first pass implementation. I think we want to avoid adding a bespoke flag for each of these options, but could be supported in typed JSON perhaps. I will discuss with TJ and see if this something we want to pursue in a follow up PR.
There was a problem hiding this comment.
I don't see a user setting a createdBy by I can see customers wanting to set branchName, commitMessage. We can do this as Follow up PR.
7a4527d to
4f027e9
Compare
| flags: [ | ||
| flag("name", "the name of the configuration bundle", z.string().optional()), | ||
| flag( | ||
| "components", |
There was a problem hiding this comment.
Should this be sensitive?
jariy17
left a comment
jariy17
left a comment
There was a problem hiding this comment.
Looks good but needs golden tests.
| @@ -0,0 +1,207 @@ | |||
| import { describe, expect, test } from "bun:test"; | |||
There was a problem hiding this comment.
Shouldn't the handler's golden tests cover these core functions?
| const current = await control.send( | ||
| new GetConfigurationBundleCommand({ bundleId: id, branchName: update.branchName }), | ||
| ); | ||
| if (!current.versionId) { |
There was a problem hiding this comment.
According to the internal service model, this is required. I don't know why aws sdk v3 has all required response parameters as | undefined.
There was a problem hiding this comment.
Yeah I saw the field was required, but the SDK type has string | undefined. I added this check to follow the service model
| @@ -0,0 +1,473 @@ | |||
| import { afterEach, describe, expect, test } from "bun:test"; | |||
There was a problem hiding this comment.
SHould this be a golden test?
| export type CreateConfigurationBundleInput = Pick< | ||
| CreateConfigurationBundleRequest, | ||
| "bundleName" | "components" | "kmsKeyArn" | ||
| >; |
There was a problem hiding this comment.
I don't see a user setting a createdBy by I can see customers wanting to set branchName, commitMessage. We can do this as Follow up PR.
jariy17
left a comment
jariy17
left a comment
There was a problem hiding this comment.
Unit tests could changed as a follow up
aidandaly24
left a comment
aidandaly24
left a comment
There was a problem hiding this comment.
Thanks for making the updates!
4 participants
Description
Adds imperative CRUDL and version listing commands for AgentCore evaluation configuration bundles:
eval config-bundle createeval config-bundle geteval config-bundle listeval config-bundle updateeval config-bundle deleteeval config-bundle version listSummary of changes
--componentsaccepts inline JSON,file://<path>, or stdin (-) viaSourceResolver.parseJsonFlagWithSchemautility.getcalls the latest-bundle API by default and the version API when optional--versionis provided.listreturns configuration bundles, whileversion listreturns all versions belonging to one bundle.updaterequires--id,--components, and--commit-message;--kms-key-arnremains optional. This counteracts the service API docs and smithy models, but is the confirmed validation structure in the service itselfparentVersionIdsentry, as required by service lineage validation.Type of Change
Testing
How have you tested the change?
Manually smoke tested all commands against a configuration bundle in my personal account:
bun run test(993 pass, 0 fail)I ran
npm run test:unitandnpm run test:integI ran
npm run typecheckI ran
npm run lintIf I modified
src/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.