Fsoc 24 #145

GDW1 · 2023-07-10T21:21:32Z

Description

Every 24 hours, FSOC will check if there is a update available. It will create a temp file that holds the most recent timestep

Type of Change

Checklist

I have read the contributing guidelines
Existing issues have been referenced (where applicable)
I have verified this change is not present in other open pull requests
Functionality is documented
All code style checks pass
New code contribution is covered by automated tests
All new and existing tests pass

…n list

cmd/root.go

codecov-commenter · 2023-07-12T21:21:50Z

Codecov Report

Merging #145 (29c082f) into main (62d908f) will decrease coverage by 0.65%.
The diff coverage is 0.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #145      +/-   ##
==========================================
- Coverage   32.34%   31.69%   -0.65%     
==========================================
  Files          27       28       +1     
  Lines        3568     3641      +73     
==========================================
  Hits         1154     1154              
- Misses       2323     2396      +73     
  Partials       91       91

Impacted Files	Coverage Δ
cmd/version/version_check.go	`0.00% <0.00%> (ø)`

... and 2 files with indirect coverage changes

pnickolov · 2023-07-15T06:44:30Z

cmd/root.go

@@ -93,6 +102,7 @@ func init() {
 	rootCmd.PersistentFlags().BoolP("verbose", "v", false, "Enable detailed output")
 	rootCmd.PersistentFlags().Bool("curl", false, "Log curl equivalent for platform API calls (implies --verbose)")
 	rootCmd.PersistentFlags().String("log", path.Join(os.TempDir(), "fsoc.log"), "determines the location of the fsoc log file")
+	rootCmd.PersistentFlags().Bool("no-update-warning", false, "Removes daily warning for updates")


Suggested change

rootCmd.PersistentFlags().Bool("no-update-warning", false, "Removes daily warning for updates")

rootCmd.PersistentFlags().Bool("no-version-check", false, "Removes daily check for new versions of fsoc")

Note that what we're skipping is the whole check, not just the warning. If this flag is set, none of the actions should be taken (no reading the timestamp file, no reading the latest version, no updating the timestamp file). This is useful in restricted environments (e.g., CI), where outgoing access is not possible or there is no need to perform the check; as well as in cases when the check is causing problems.

pnickolov · 2023-07-15T06:48:08Z

cmd/root.go

@@ -222,6 +232,35 @@ func preExecHook(cmd *cobra.Command, args []string) {
 			log.Fatalf("fsoc is not configured, please use \"fsoc config set\" to configure an initial context")
 		}
 	}
+
+	// Do version checking
+	noUpdateWarning, _ := cmd.Flags().GetBool("no-update-warning")


Also please check if FSOC_NO_VERSION_CHECK (matches the flag name, prefixed with FSOC_ and uppercased) environment variable is set. For some cases, such as CI, it is more convenient to set the env var once rather than add the flag to every command.

pnickolov · 2023-07-15T06:49:11Z

cmd/root.go

+
+	// Do version checking
+	noUpdateWarning, _ := cmd.Flags().GetBool("no-update-warning")
+	updateChecked := int(time.Now().Unix())-getRecentTimestamp() > int(secondInDay) && !noUpdateWarning


All this code must be conditional on whether we're doing the check. If the --no-version-check flag is set (or env var), there is no need to check timestamps. You can leave the channel as nil, and then in the post hook, omit reading if it's nil

pnickolov · 2023-07-15T06:54:37Z

cmd/root.go

+	_ = os.Remove(os.TempDir() + "fsoc.timestamp")
+	_, err = os.Create(os.TempDir() + "fsoc.timestamp")
+	if err != nil {
+		log.Errorf(err.Error())


This should be a warning and there should be some context to the error, such as "failed to create version check timestamp file, check will be performed next time", and then the error info. This way a warning that says that, for example, a directory is not writeable, will be in context of what we were trying to do; otherwise, users will wonder why we are trying to write to a file when they are asking for a solution list, for example.
Also, when formatting an error, it's recommended to use "%w" format and the err value itself, rather than just the string err.Error(). This allows for unwrapping the error).

pnickolov · 2023-07-15T06:55:41Z

cmd/root.go

+	}
+}
+
+func getRecentTimestamp() int {


Suggested change

func getRecentTimestamp() int {

func getLastVersionCheckTime() int {

pnickolov · 2023-07-15T07:16:51Z

cmd/version/version.go

+	currentVersion := GetVersion()
+	currentVersionSemVer := ConvertVerToSemVar(currentVersion)
+	newerVersionAvailable := currentVersionSemVer.Compare(newestVersionSemVar) == -1
+	var debugFields = log.Fields{"newerVersionAvailable": newerVersionAvailable, "oldVersion": currentVersionSemVer.String(), "newVersion": newestVersionSemVar.String()}


I don't think we should include the newerVersionAvailable field. This should be--and is--part of the message: a newer version is available.
Also, instead of "old" and "new" version field names, it will be more helpful to call them "current" (i.e., this is what your current fsoc that is running is) and "latest" (i.e., this is what is available / you should run)

pnickolov · 2023-07-15T07:17:47Z

cmd/version/version.go

+	if newerVersionAvailable {
+		log.WithFields(debugFields).Warnf("There is a newer version of FSOC available, please upgrade from version %s to version %s", currentVersionSemVer.String(), newestVersionSemVar.String())
+	} else {
+		log.WithFields(debugFields)


I don't think we need to log this; but it we do, I propose we just log the version values

pnickolov · 2023-07-15T07:18:36Z

cmd/version/version.go

+	var debugFields = log.Fields{"newerVersionAvailable": newerVersionAvailable, "oldVersion": currentVersionSemVer.String(), "newVersion": newestVersionSemVar.String()}
+
+	if newerVersionAvailable {
+		log.WithFields(debugFields).Warnf("There is a newer version of FSOC available, please upgrade from version %s to version %s", currentVersionSemVer.String(), newestVersionSemVar.String())


Since we already include the versions in the fields, I don't think we need to include them in the message as well. Just say "There is a newer version of fsoc available, please upgrade soon"

pnickolov · 2023-07-15T07:20:08Z

cmd/version/version.go

consider moving this to a versioncheck.go. Even though the file is short, it is quite dense and I think it will be better to separate the code related to the latest-version check.

pnickolov · 2023-07-15T07:21:50Z

cmd/root.go

+	if updateChecked {
+		go version.CheckForUpdate(updateChannel)
+	} else {
+		go func() { updateChannel <- version.ConvertVerToSemVar(version.GetVersion()) }()


If the purpose of this is just to not do (skip) the check, isn't there a better way?

pavel-georgiev · 2023-07-15T21:23:38Z

@pnickolov it is great to have the ability to check for available updates. Can we make this to not be the default behavior. This kind of hidden work raises some questions, in most cases would be undesirable and AFAIK it is somewhat unorthodox for an OSS CLI tool. On top of being a bad practice (IMO), I have a few practical cases where it would be undesirable:

I do a local operation (like fsoc solution bump), but now I have to wait for a network call for something that I did not ask for
Version check fails (or release location changed) - is the exit status going to reflect the status of my operation or the version check?

Here are some suggestions on how to make it less intrusive, ordered by my preference:

Make this run only as part of fsoc version so it can show you your version and latest available version, keep the flag for disabling the remote check
Make this a "best effort" operation - if it is done by the time the command is done, then tell me about the new version (and/or store the result so you can keep warning me even without making the network call), if it is not done, try next time. In short - never make fsoc run longer because of the version check
Have a config option to disable this (I suspect it will be a popular option, especially for CI operations and internal)

pnickolov · 2023-07-17T08:03:04Z

In reply to @pavel-georgiev : I agree with some of the issues but we still expect significant changes and making sure the latest version is in use is important. We're trying to follow the pip approach (except warn no more than once a day).

I think @GDW1 has addressed most of the issues:

the check is best effort; if it fails, it will be warned but the operation and its exit code are never affected
the version check is done in parallel with the main operation
while very fast operations will still wait for the version check to complete at exit, the check is performed no more often than once a day, so it will not affect most operations (only one per day, less than login)
there is a global command-line option AND an environment variable to disable the version check (for CI or in environments that don't have access to github or writeable temp dir)

GDW1 added 4 commits June 29, 2023 16:03

Filter and Print options for queries. example shown with fsoc solutio…

4b8a74c

…n list

version checking draft

244053e

Warn if upgrade is avilable every 24 hours

a92a4c8

Warn if upgrade is avilable every 24 hours

1e59827

GDW1 requested a review from pnickolov as a code owner July 10, 2023 21:21

GDW1 added 4 commits July 10, 2023 14:24

Warn if upgrade is avilable every 24 hours

97be983

Warn if upgrade is avilable every 24 hours

3ad4193

Warn if upgrade is avilable every 24 hours

05b413d

Warn if upgrade is avilable every 24 hours

62a47b5

pnickolov requested changes Jul 12, 2023

View reviewed changes

refactored the versioning and the where the timestamps are stored

248e809

GDW1 added 5 commits July 12, 2023 14:53

Async checking

877dddc

make pre-commit

c998ba4

make pre-commit

78722fc

make pre-commit

6e37c5d

make pre-commit

36a3dda

pnickolov requested changes Jul 15, 2023

View reviewed changes

GDW1 added 2 commits July 15, 2023 09:34

version code moved to difference files

126b780

version code moved to difference files

29c082f

pnickolov approved these changes Jul 17, 2023

View reviewed changes

pnickolov merged commit b177e7f into cisco-open:main Jul 17, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fsoc 24 #145

Fsoc 24 #145

GDW1 commented Jul 10, 2023

codecov-commenter commented Jul 12, 2023 •

edited

Loading

pnickolov Jul 15, 2023

pnickolov Jul 15, 2023

pnickolov Jul 15, 2023

pnickolov Jul 15, 2023

pnickolov Jul 15, 2023

pnickolov Jul 15, 2023

pnickolov Jul 15, 2023

pnickolov Jul 15, 2023

pnickolov Jul 15, 2023

pnickolov Jul 15, 2023

pnickolov Jul 15, 2023

pavel-georgiev commented Jul 15, 2023 •

edited

Loading

pnickolov commented Jul 17, 2023 •

edited

Loading

	rootCmd.PersistentFlags().Bool("no-update-warning", false, "Removes daily warning for updates")
	rootCmd.PersistentFlags().Bool("no-version-check", false, "Removes daily check for new versions of fsoc")

	func getRecentTimestamp() int {
	func getLastVersionCheckTime() int {

Fsoc 24 #145

Fsoc 24 #145

Conversation

GDW1 commented Jul 10, 2023

Description

Type of Change

Checklist

codecov-commenter commented Jul 12, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-georgiev commented Jul 15, 2023 • edited Loading

pnickolov commented Jul 17, 2023 • edited Loading

codecov-commenter commented Jul 12, 2023 •

edited

Loading

pavel-georgiev commented Jul 15, 2023 •

edited

Loading

pnickolov commented Jul 17, 2023 •

edited

Loading