-
Notifications
You must be signed in to change notification settings - Fork 26
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
|
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
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? |
| ) | ||
| fab_ui.print_grey(msg) | ||
| elif latest_version: | ||
| fab_logger.log_debug(f"Already on latest version: {__version__}") |
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.
why did you used fab_logger.log_debug and not fab_ui.print_grey? using fab_logger.log_debug means user will see it only if fab_constant.FAB_DEBUG_ENABLED is set by the user to true.
Also, are those scenarios tested?
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'm using fab_logger.log_debug here intentionally for the "already up to date" scenario. The rationale is:
- Silent success when the user is already on the latest version.
- User-facing notification only when actionable (in case of a newer version available)
100% coverage on fab_version_check.py:
Name Stmts Miss Branch BrPart Cover Missing
---------------------------------------------------------------------------------------
src/fabric_cli/utils/fab_version_check.py 32 0 6 0 100%
---------------------------------------------------------------------------------------
TOTAL 32 0 6 0 100%
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.
agree for the use of fab_logger.log_debug
regarding tests - my Q was that if we verify the correct message is printed only when debug_enabled is true
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.
It was already tested before by this function, however I made it a bit more explicit by adding mock_questionary_print.assert_not_called().
fabric-cli/tests/test_utils/test_fab_version_check.py
Lines 164 to 175 in b56ef8e
| @patch("fabric_cli.utils.fab_version_check._fetch_latest_version_from_pypi") | |
| def test_cli_version_check_disabled_by_config_success( | |
| mock_fetch, mock_fab_set_state_config, mock_questionary_print | |
| ): | |
| """Should not display notification when update checks are disabled by config.""" | |
| newer_version = _increment_version("major") | |
| mock_fab_set_state_config(fab_constant.FAB_CHECK_UPDATES, "false") | |
| mock_fetch.return_value = newer_version | |
| fab_version_check.check_and_notify_update() | |
| mock_questionary_print.assert_not_called() |
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.
What’s missing is a check that the correct message is logged when debug_enabled=True in test_cli_version_check_same_version_success. You verified mock_questionary_print.assert_not_called(), but you should also assert that log.debug was called with the expected message.
use mock_fab_set_state_config(constant.FAB_DEBUG_ENABLED, True) to set debug_enabled=True then mock fab_logget.debug and verify the message
Same for: "test_cli_version_check_fetch_failure" and "test_cli_version_check_older_version_success"
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.
Apologies, I didn't know that you were referring to testing the logger outputs. I fixed it in b384c46
Fixes auth test failures caused by version check requests not present in existing recordings. PyPI checks are tested separately.
14742d2 to
e3a65b1
Compare
2857200 to
b56ef8e
Compare
Co-authored-by: aviatco <32952699+aviatco@users.noreply.github.com>
a5f498d to
b384c46
Compare
📥 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_cli_version_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: