-
Notifications
You must be signed in to change notification settings - Fork 24
feat: inform user about cli updates #91
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?
feat: inform user about cli updates #91
Conversation
|
|
||
|
|
||
| class TestFetchLatestVersionFromPyPI: | ||
| """Test _fetch_latest_version_from_pypi logic.""" |
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.
Please combine the classes to one TestVersionCheck
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.
I will leave out the use of test classes entirely. I see that other test files also do not use them unless there is a reason for it (shared fixtures or setup/teardown). In my case I did more of a logical grouping of different parts but I think it makes less sense now especially since the caching part is removed.
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.
done in 8e3e69e
| """Test _fetch_latest_version_from_pypi logic.""" | ||
|
|
||
| @patch("fabric_cli.utils.fab_version_check.requests.get") | ||
| def test_fetch_success(self, mock_get): |
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.
Please use more meaningfull name such as: test_cli_version_fetch_success / test_cli_version_fetch_failure
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.
done in 8e3e69e
|
|
||
| @patch("fabric_cli.utils.fab_version_check.requests.get") | ||
| def test_fetch_failure_http_error(self, mock_get): | ||
| """Should return None when PyPI returns non-200 status code.""" |
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.
please align to the naming convention and use failure/success as suffix.
In other tests as well
|
Thanks for the review @aviatco. Your points are valid and I will keep them in mind in my potential future PRs. I left some of the remarks open (like proper naming and the class one) as these warrant a second review by you to verify if my changes are meeting your request. |
ccf89bf to
f506576
Compare
Fixes auth test failures caused by version check requests not present in existing recordings. PyPI checks are tested separately.
f4b4c4e to
13acee7
Compare
|
@aviatco @ayeshurun in f4b4c4e I made a change to ignore pypi.org in the VCR playback. This was causing issues in the This passes the tests however now during testing of the fab auth login, actual requests to PyPI are being made. I think it would be cleaner to either:
Could you provide some guidance here? |
📥 Pull Request
✨ Description of new changes
Implements automatic update notifications that inform users when a new version of the Fabric CLI is available on PyPI. The check runs on login (
fab auth login) and displays a friendly notification if an update is available.Changes Made
New Files:
src/fabric_cli/utils/fab_version_check.py- Core version checking module with PyPI integrationtests/test_utils/test_fab_version_check.py- 100% coverage on fab_version_check.pyModified Files:
src/fabric_cli/core/fab_constant.py- Added constants for update checking configurationsrc/fabric_cli/commands/auth/fab_auth.py- Integrated version check on successful logindocs/essentials/settings.md- Documented the newcheck_updatessettingImplementation Details
fab auth loginfab config set check_updates falsefab_loggerfor troubleshootingConfiguration
New config key
check_updates(default:true) stored in~/.config/fab/config.json. This setting enables/disables update notifications.User Experience
When a user logs in and a new version is available: