-
Notifications
You must be signed in to change notification settings - Fork 20
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
Notify user of a new recommended CLI version #707
Notify user of a new recommended CLI version #707
Conversation
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 am still reviewing the PR but adding some early comments.
a649dde
to
93441c7
Compare
93441c7
to
ac7f42a
Compare
Based on a few comments, it might be good to understand procedurally how time-sensitive it is with regards to when to publish a CLI version and when to publish the recommendation data in the Central Configuration. Maybe update the PR description with this info? |
I was thinking about For testing, where we need a much smaller delay, instead of adding another variable, I was thinking of using negative values to indicate seconds, so Let me know if you disagree. |
ee9acce
to
9174c53
Compare
I have added a One scenario that may not be obvious is for pre-releases: If the pre-release is for a new patch version, then the recommended version of the previous patch should NOT be removed, and the pre-release should be added as a new recommended entry in the central config. For example, if |
180eb43
to
5c2c579
Compare
I've updated the API for the new data store to use the same approach than the Central Configuration API (marshall/unmarshall the value stored) as it is a nicer approach. I have also added documentation about:
|
5f053bd
to
131e6ae
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.
Looks good to me. This is a really nice set of changes, @marckhouzam !
A few things:
I assume you plan to do some rebase/squashing. If you do, please retain a handful of logical commits so it will be easier for future reference.
Also, something to consider as a followup:
Over time, it will become more and more important that a CLI has up-to-date information about the central configuration, else might start to exhibit different/possibly unwanted behavior.
The situation that is really important to get right (though it is likely not the only one) is when the user picks up a Central-Config aware CLI for the first time.
I suggest that any caching behavior we have today with regards to the fetching and updating of the data from the centrl OCI image be examined.
The conservative thing to do would be to invalidate the cache on any new CLI version.
Few options if we are to do this:
track in the data store file any CLI version(s) that the CLI host has seen, and perform any cleanup/invalidation (and update of the data store file with the new version, of course) if we encounter a new CLI version.
Alternatively, store in the cache the version of the CLI responsible for populating it and treat any new cli version as an indication that the cache should be invalidate.
There could be more uncommon situations where the cache data needs to be invalidated as well. One could be when a discovery source is updated. It is less likely to happen in production, but might be worth verifying as well.
This can be used by the CLI to store non-configuration data. Signed-off-by: Marc Khouzam <marc.khouzam@broadcom.com>
Signed-off-by: Marc Khouzam <marc.khouzam@broadcom.com> Co-authored-by: Anuj Chaudhari <anuj.chaudhari@broadcom.com>
e3af951
to
90846fe
Compare
This commit has the CLI read the Central Configuration and look for the different recommended versions of the CLI itself. If the user is using an older version than the recommended version, a notification will be printed to the screen but at most every 24 hours. This commit includes E2E tests and documentation. Signed-off-by: Marc Khouzam <marc.khouzam@broadcom.com>
90846fe
to
65a38d2
Compare
Thank you @vuil and @anujc25 for working with me to reach this final iteration!
Good point I have squashed on my machine and kept the following three commits (most recent at the top of the list):
Excellent point! |
What this PR does / why we need it
This PR may seem very big but notice it has 1750 lines of new unit/e2e tests.
This PR teaches the CLI to detect a new release of the CLI itself and notify the user they should upgrade.
The notification is printed at most once every 24 hours by default.
The PR also implements a yaml data-store that the CLI can use to store different information that does not belong in the configuration file. This new file is located in
$HOME/.config/.data-store.yaml
. For this feature, the CLI stores the timestamp of the last time the user was notified to upgrade.The PR also implements an API to read the Central Configuration obtained from the Central Repository of plugins, as introduced in #700. For this feature, the Central Configuration is read to detect any new version of the CLI having been released. The PR also updates the test central repo to improve the format of the central config.
Things to know:
Major,Minor and Patch (see the testing section below for an example)In practice, there should never be a line for Major since we expect all users to be using some.v1
version. If a user is still using thev0.90
version, then the line for the MAJOR version will be shownv0.90.1
) so in most cases there will be a single line showing the new minortanzu __complete
,tanzu completion
andtanzu version
stderr
so that it does not affect potential parsing of the real command outputTANZU_CLI_RECOMMEND_VERSION_DELAY_DAYS
can be used to change the period of the notification (default 1 day) or to disable the feature by setting it to0
. If this variable is set to a negative number then the delay will be in seconds instead of days; this allows for testing.cli.core.cli_recommended_versions
in the central config is changed from a comma-separated list to a yaml array of structs. With the latest central config API this is easier to parse, it is also easier for a human to read, and it even allows to add comments. The use of structs makes it future-proof as new fields can be added in the future without breaking existing CLIs.When to update the central config
Whenever there is a new release of the CLI, be it major, minor, patch or even pre-release, we should add the new version to the production Central Config in the existing array of key
cli.core.cli_recommended_versions
and publish a new image of the production central repo. There is no rush to do it but no message about a new version will be printed until it is published. We probably only want to publish once the version is GA (except for pre-releases).Real scenarios
When v1.3.0 is released with this feature
No notification will be printed since existing CLIs (pre
v1.3.0
) don't have this feature.A notification of a new recommended version will only happen once we release a new minor or patch after the
v1.3.0
version.The central config should still be updated with the list of recommended version:
A new minor release of the CLI is published
As soon as the release is available, users can install it. However, no new message will be shown until we update the Central Configuration with this new version.
If a user installs the new version, no message will be printed either although the current central config recommends an older version, it is for this use case that we don't print a message to downgrade to an older version, as it would not be valid in this case.
When GA happens we then add the new minor version to the production Central Config in the existing array of key
cli.core.cli_recommended_versions
and publish a new image of the production central repo. At this time a running older CLI will notice the new version BUT only when it refreshes its central repo data, which will happen at most within 24 hours of usage (thanks to the essential plugins threshold of #637); once the CLI refreshes its central repo it will print the notification of the new version, at most once every 24 hours.A new patch release of the CLI is published
Same as for a minor except that in the central config, the existing patch version of the same minor should be replaced. For example if the central config recommends
v1.3.0
and we releasev1.3.1
, then the entryv1.3.0
should be replaced byv1.3.1
.A new major release of the CLI is published
Same as for a minor.
Note that although the CLI won't recommend a new major, we do want to include the new major in the central config so that people that start using that major can eventually get notifications of a new minor/patch for that major.
A new pre-release of the CLI is published
Depending on if this pre-release is for a new minor or not, the pre-release should be added (new minor) or updated (existing minor) into the central config.
If the pre-release is for a new patch version, then the recommended version of the previous patch should NOT be removed, and the pre-release should be added as a new recommended entry in the central config. For example, if
v1.3.4
is a recommended version and av1.3.5-alpha.1
is released, then thev1.3.4
entry should not be removed until the finalv1.3.5
is available; thev1.3.5-alpha.1
version should be added as a new entry in the central config.No need to wait for GA for a pre-release.
Which issue(s) this PR fixes
Fixes #706
Describe testing done for PR
Added unit tests. and E2E tests.
Manual testing:
Setup. Notice the recommended versions that are part of the test central repo (at the end of the setup):
Run a CLI with a version that is not recommended:
Run the CLI with a version that is the latest minor but not the latest patch, and notice only the newer patch is recommended:
Test that nothing is recommended if the recommended version is older
Test the configuration of the notification delay
Run a CLI with a pre-release version and notice that pre-release recommendations are now shown:
Check the 3 commands that don't trigger the check and notice there is no notification:
Check that the notification is on
stderr
:Release note
Additional information
Special notes for your reviewer
If you want to test this, you will need to use the latest test central repo.
If you haven't yet, don't forget to cleanup your test central repo cache to get the latest one: