Skip to content
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

Improve update checker #122

Closed
kadel opened this issue Feb 22, 2018 · 20 comments
Closed

Improve update checker #122

kadel opened this issue Feb 22, 2018 · 20 comments
Assignees
Labels
estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person priority/Low Nice to have issue. It's not immediately on the project roadmap to get it done.

Comments

@kadel
Copy link
Member

kadel commented Feb 22, 2018

Current implementation noticeably slows ocdev execution even on a really fast internet connection.

We definitely should have an option to disable this check permanently. We could have something like ocdev config set checkForUpdates false and use config file to persist configuration.

We could also perform check only once a day (use config file to persist when check occurred last time)
we can do those in separate PR, but think that we definitely have to add a way to disable it.

Another option could be to perform check as async operation.


Update: 2018-07-12

Summary:

There are two things that are needed.

First

We need a configuration uption that will disable update checker.

There should be new commadn odo config

# disable update notification
odo config set UpdateNotification false

# view current value
odo config get UpdateNotification

UpdateNotification = false

Value of UpdateNotification is saved in config file.
There is Config struct in config.go that represent ODO config file structure. (

type Config struct {
)
There should be a new field called UpdateNotification.
odo config set UpdateNotification false sets value of this field to false, (or true if odo config set UpdateNotification true is called)

Update checker should be started only when this configuration field is set to true.
Default value if the configuration is not specified is true

Second

We need a better way to check for updates that don't require calling GitHub API.

TBD

@kadel kadel added kind/task priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). labels Feb 22, 2018
@concaf
Copy link
Contributor

concaf commented Feb 26, 2018

Update: @baijum gave some nice pointers on how to solve this in https://gist.github.com/baijum/036a79ec2e85279af596d0a2a3e0e13e, continuing work.

concaf added a commit to concaf/odo that referenced this issue Feb 26, 2018
Prior to this commit, the latest release information was fetched
from GitHub in a blocking manner, which means that even for a very
quick command like `ocdev version`, the tool would wait for a few
seconds before outputting the version, because the execution would
be blocked on waiting for the response from GitHub APIs.

This commit changes that and makes the request to GitHub
concurrent and non-blocking. This means that if the request has
not completed but the program execution has ended, then the tool
will exit gracefully without waiting for the request to complete.

However, if the program execution took longer than the request and
the response has been received by the goroutine before the program
execution is over, then, if applicable, the information is
printed.

Fixes redhat-developer#122
concaf added a commit to concaf/odo that referenced this issue Feb 27, 2018
Prior to this commit, the latest release information was fetched
from GitHub in a blocking manner, which means that even for a very
quick command like `ocdev version`, the tool would wait for a few
seconds before outputting the version, because the execution would
be blocked on waiting for the response from GitHub APIs.

This commit changes that and makes the request to GitHub
concurrent and non-blocking. This means that if the request has
not completed but the program execution has ended, then the tool
will exit gracefully without waiting for the request to complete.

However, if the program execution took longer than the request and
the response has been received by the goroutine before the program
execution is over, then, if applicable, the information is
printed.

Fixes redhat-developer#122
concaf added a commit to concaf/odo that referenced this issue Feb 27, 2018
Prior to this commit, the latest release information was fetched
from GitHub in a blocking manner, which means that even for a very
quick command like `ocdev version`, the tool would wait for a few
seconds before outputting the version, because the execution would
be blocked on waiting for the response from GitHub APIs.

This commit changes that and makes the request to GitHub
concurrent and non-blocking. This means that if the request has
not completed but the program execution has ended, then the tool
will exit gracefully without waiting for the request to complete.

However, if the program execution took longer than the request and
the response has been received by the goroutine before the program
execution is over, then, if applicable, the information is
printed.

Fixes redhat-developer#122
concaf added a commit to concaf/odo that referenced this issue Feb 28, 2018
Prior to this commit, the latest release information was fetched
from GitHub in a blocking manner, which means that even for a very
quick command like `ocdev version`, the tool would wait for a few
seconds before outputting the version, because the execution would
be blocked on waiting for the response from GitHub APIs.

This commit changes that and makes the request to GitHub
concurrent and non-blocking. This means that if the request has
not completed but the program execution has ended, then the tool
will exit gracefully without waiting for the request to complete.

However, if the program execution took longer than the request and
the response has been received by the goroutine before the program
execution is over, then, if applicable, the information is
printed.

Fixes redhat-developer#122
@kadel kadel reopened this Feb 28, 2018
@kadel
Copy link
Member Author

kadel commented Feb 28, 2018

reopening

Let's close this after we implement option to completely disable checking for updates

@kadel
Copy link
Member Author

kadel commented Feb 28, 2018

we could implement configuration subcommand similar to minikube:

# disable update notification
ocdev config set UpdateNotification false


# view current value
ocdev config get UpdateNotification
UpdateNotification = false

@kadel
Copy link
Member Author

kadel commented May 29, 2018

Also, we are fetching latest information from GitHub API. Problem is that this will hit Github API rate limiting after a few odo runs. We should check for latest release number in some other way.

@kadel kadel added priority/Low Nice to have issue. It's not immediately on the project roadmap to get it done. and removed priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). labels May 30, 2018
@surajnarwade
Copy link
Contributor

@kadel instead of hitting github API, can we have file like this, https://github.com/openshift/origin/blob/master/.release which we will update at every release ?

@kadel
Copy link
Member Author

kadel commented Jun 8, 2018

We already have a version number in https://github.com/redhat-developer/odo/blob/master/cmd/version.go could we used that?
Or modify version.go to read .release?

@kadel
Copy link
Member Author

kadel commented Jul 18, 2018

@syamgk what is the status of this?

@syamgk
Copy link
Member

syamgk commented Jul 18, 2018

working will push the initial commit today

@syamgk
Copy link
Member

syamgk commented Jul 19, 2018

WIP PR can be found here #589

@syamgk
Copy link
Member

syamgk commented Jul 23, 2018

also #593 for eliminate calling GitHub API for version fetching

@jorgemoralespou
Copy link
Contributor

Please, @kadel and all, don't use this signature for configuration of the CLI itself. This signature should be used for configuration of a component, meaning adding configuration to it in the form of an environment variable or a file, whether internally as a secret or configmap or value directly on the deployment.

For CLI configuration I would propose one of the following alternatives:

  • $ odo option set timeout 2
  • $ odo adm option set timeout 2
  • $ odo adm config set timeout 2

And use set/unset/clear as verbs, instead of "create,update,delete"

@kadel
Copy link
Member Author

kadel commented Aug 13, 2018

@jorgemoralespou You are right, it makes much more sense to reserve odo config for component configuration.

I don't like odo adm to be honest. It just looks a bit weird to me.

We have odo utils command. (odo utils completion and odo utils terminal).
Would it make sense to have odo utils option?
Or we could rename utils to something better more suitable. It would be nice to have one common root command under which we can put all non component/application related stuff.

@jorgemoralespou
Copy link
Contributor

I never liked utilsmyself :-D but yes, should probably belong in the same "option namespace" so whatever we decide as naming for this, should potentially all go there.

@gshipley @marekjelen any idea on how to name this?

@marekjelen
Copy link

I am pretty much OK with the utils, as the name imho represents what’s in the namespace.

For the tool config option seems like a good candidate. setup might work, but it may not be as intuitive.

@syamgk
Copy link
Member

syamgk commented Aug 16, 2018

so renaming it from config and configuration to utils and utility on my PR

@kadel
Copy link
Member Author

kadel commented Aug 17, 2018

so renaming it from config and configuration to utils and utility on my PR

we can leave it as utils, utility is just more letters to type :-)

commands should be odo utils option set/odo utils option get or odo utils config set/odo utils config get

@Jessica6552
Copy link

Humm

@syamgk
Copy link
Member

syamgk commented Sep 5, 2018

closing this issue since these three pr's got merged
#613
#593
#589

@syamgk syamgk closed this as completed Sep 5, 2018
@rm3l rm3l added the estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person label Jun 18, 2023
@rm3l rm3l added this to odo Project Jun 18, 2023
@rm3l rm3l moved this to Done ✅ in odo Project Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person priority/Low Nice to have issue. It's not immediately on the project roadmap to get it done.
Projects
Archived in project
Development

No branches or pull requests

8 participants