-
Notifications
You must be signed in to change notification settings - Fork 366
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)
genkit-tools/cli/src/utils/global.ts
Outdated
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
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
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 update
command 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(``); |
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'); |
Copilot uses AI. Check for mistakes.
} | ||
|
||
const alternativeCommand = `curl -Lo ./genkit_bin ${downloadUrl}`; | ||
logger.info(``); |
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'); |
Copilot uses AI. Check for mistakes.
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):