-
Notifications
You must be signed in to change notification settings - Fork 623
feat(genkit-tools/cli): add update cmd #3389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…tase/cli-add-update-cmd
…al install, inquire it
…error message when installing same version with -v (must use -r)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename this file? global doesn't feel descriptive/accurate
cabljac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments.
Didn't we downscope to just printing appropriate commands for them to use if installed via npm?
|
@cabljac I believe we downscoped from detecting the package manager to prompting what the user uses as a package manager. |
a486db8 to
8496f24
Compare
cabljac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments
| // Get all version numbers and sort them | ||
| const versions = Object.keys(data.versions); | ||
|
|
||
| // Filter out pre-release versions and sort by semantic version (newest first) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use the package semver here instead of manual string parsing?
I think it will handle comparisons and prerelease stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces an update command for the Genkit CLI that enables users to check for and install updates to the CLI tool. The implementation supports both npm-based installs (Node.js runtime) and binary installs, with automatic update notifications and configurable options.
Key changes:
- Adds
genkit updatecommand with options for checking updates, reinstalling, and specifying versions - Implements automatic update notifications that can be disabled via configuration or environment variable
- Supports both npm registry (for Node.js) and Google Cloud Storage (for binaries) as update sources
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| genkit-tools/cli/src/commands/update.ts | Main update command implementation with version checking and installation logic |
| genkit-tools/cli/src/utils/package-manager.ts | Package manager abstraction for npm, pnpm, and yarn support |
| genkit-tools/cli/src/utils/utils.ts | Custom UpdateError class for error handling |
| genkit-tools/cli/src/commands/config.ts | Adds configuration option to disable update notifications |
| genkit-tools/cli/src/cli.ts | Integrates update notification into CLI entry point |
| genkit-tools/cli/tests/commands/update_test.ts | Comprehensive test suite for update functionality |
| genkit-tools/cli/package.json | Updates genversion script to include package name |
Comments suppressed due to low confidence (2)
genkit-tools/cli/src/commands/update.ts:1
- This mock is duplicated on lines 20-27 and 84-87 in the test file. The second mock will override the first one, making the initial mock configuration ineffective.
/**
genkit-tools/cli/src/commands/update.ts:1
- Calling mockRestore() on a jest.fn() mock will throw an error since jest.fn() mocks don't have a restore method. Use mockReset() or mockClear() instead, or create the mock with jest.spyOn() if you need restore functionality.
/**
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| `${clc.green('✓')} Successfully updated to v${clc.bold(version)}` | ||
| ); | ||
| } catch (error: any) { | ||
| logger.info(``); |
Copilot
AI
Aug 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line logs an empty string which appears to be used for spacing. Consider using a more explicit approach like logger.info('\n') or removing this line if it's unnecessary.
| logger.info(``); | |
| logger.info('\n'); |
| } | ||
|
|
||
| const alternativeCommand = `curl -Lo ./genkit_bin ${downloadUrl}`; | ||
| logger.info(``); |
Copilot
AI
Aug 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line logs an empty string which appears to be used for spacing. Consider using a more explicit approach like logger.info('\n') or removing this line if it's unnecessary.
| logger.info(``); | |
| logger.info('\n'); |
1e20f9b to
a3ca3d0
Compare
|
|
||
| if (result.hasUpdate) { | ||
| // Merge all notification lines into a single message for concise output | ||
| console.log( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| console.log( | |
| console.error( |
bf12bdf to
a3ca3d0
Compare
Checklist (if applicable):