feat: add ai:models:detach command
#18
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sbosio
approved these changes
Sep 26, 2024
src/commands/ai/models/detach.ts
Outdated
| const resource = error.http.body.resource | ||
|
|
||
| if (statusCode === 404 && resource === 'attachment') { | ||
| ux.error(`We can’t find a model resource called ${modelResource}. Run 'heroku addons' to see a list of model resources attached to your app.`) |
There was a problem hiding this comment.
We agreed with Sandy to use Run 'heroku ai:models:info -a <appname' to see a list..., because it's the command expected to return all model resources according to the requested UX.
There was a problem hiding this comment.
Ah ok. That change must not have made it into the design doc. Thanks.
sbosio
reviewed
Sep 26, 2024
| await runCommand(Cmd, ['--app', 'myapp', 'model-123']) | ||
|
|
||
| expect(stdout.output).to.equal('') | ||
| expect(stderr.output).to.contain('Detaching model-123 to model from myapp... done\n') |
There was a problem hiding this comment.
Using the stripAnsi helper might help with the differences between local and CI.
There was a problem hiding this comment.
I tried that here and it didn't work because depending on the connection you can get those messages multiple times. So I left it as is, but I did use the stripAnsi helper on another test below where the message got longer.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR implements the
ai:models:detachcommand with tests according to the design outlined in the UX design doc.How to test
Prepare your environment for testing
yarn && yarn build.HEROKU_INFERENCE_ADDONto the production canary add-on viaexport HEROKU_INFERENCE_ADDON="inference-staging"Actual testing
./bin/run ai:models:detach --helpheroku apps:create test-cli-plugin-ai./bin/run ai:models:create claude-3-opus -a test-cli-plugin-ai --as opus_modeland confirm that the command succeeds, creating the model resource config vars using the alias indicated.inference-staging-<adjective>-XXXXX) in the following command to create a second attachment to the same model resource:heroku addons:attach <addon-name> -a test-cli-plugin-ai --as opus_again../bin/run ai:models:detach <add-on name> -a test-cli-plugin-ai, you should get an error saying that there are multiple matching attachments./bin/run ai:models:detach opus_again -a test-cli-plugin-ai, this operation should be successful./bin/run ai:models:detach opus_false -a test-cli-plugin-ai, you should get an error that says "We can’t find a model resource called opus_false. Run 'heroku addons' to see a list of model resources attached to your app."./bin/run ai:models:detach opus_model -a false-test-app, you should get an error that says "We can’t find the false-test-app app. Check your spelling."heroku apps:destroy -a test-cli-plugin-aiSOC2 Compliance
GUS Work Item