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

ENH: use etelemetry to check for a newer version available upon running a command or importing .api #493

Closed
wants to merge 1 commit into from

Conversation

yarikoptic
Copy link
Member

It already helped me to realize that I forgot to "release" 0.2.* on Github.
Now they are released, so I guess it takes etelemetry service some time to update
its knowledge. Hopefully tomorrow we should see the effect.

Attn @satra who might provide recommendation and also I wonder if it would be worth trying to generalize such check_available_version and ship it within etelemtry-client itself, so users did not have to reimplement similar logic. Could have signature like

check_available_version(github_project, current_version, info, debug)

where info and debug will be the callable (with the same signature as of the logger) to use. What do you think @satra ?

TODOs:

  • Wrap the entire body to be ran in a separate thread. Even though it might not be taking long, IMHO it might be of benefit to cmdline apps to not spend time waiting

…ng a command or importing .api

It already helped me to realize that I forgot to "release" 0.2.* on Github.
Now they are released, so I guess it takes etemetry service some time to update
its knowledge.  Hopefully tomorrow we should see the effect.
@satra
Copy link

satra commented Dec 4, 2019

generalize such check_available_version and ship it within etelemtry-client itself, so users did not have to reimplement similar logic.

i think that would be good.

@yarikoptic
Copy link
Member Author

generalize such check_available_version and ship it within etelemtry-client itself, so users did not have to reimplement similar logic.

i think that would be good.

oh, and you have a similar one in nipype (which you pointed out in nipy/heudiconv#392)
https://github.com/nipy/nipype/blob/master/nipype/__init__.py#L69 which also adds ability to prescribe some versions as bad_versions, i.e. carrying a critical bug I guess, so then info level should become a warning.

@yarikoptic yarikoptic mentioned this pull request Dec 5, 2019
2 tasks
@kyleam
Copy link
Contributor

kyleam commented Dec 5, 2019

From a user perspective, I very much dislike a tool or library reaching out to a server each time it is invoked, for activity unrelated to anything I explicitly requested or wanted. Please consider making this opt-in or at least opt-out.

@satra
Copy link

satra commented Dec 5, 2019

From a user perspective, I very much dislike a tool or library reaching out to a server each time it is invoked, for activity unrelated to anything I explicitly requested or wanted.

@kyleam - don't you do that every time you use the browser or any app on your phone?

@kyleam
Copy link
Contributor

kyleam commented Dec 5, 2019

@kyleam - don't you do that every time you use the browser

Using a browser is a very specific request by me to connect to servers. We're talking about a local python library/command-line tool that does not even require an internet connection for many operations. I should be able to use a local tool on my computer without it phoning a server.

@satra
Copy link

satra commented Dec 5, 2019

but your apps are local and they do tell you updates are available. i think providing a way for developers to communicate with their users to notify bad versions, updates is a good thing.

i understand the philosophy, and ET does allow a way to disable it and handles no connection uses, but i am saying that most apps today communicate with servers. i don't necessarily see that as concern if we ensure that people know that the app (even if it's a python tool on the command line) will communicate.

@yarikoptic
Copy link
Member Author

From a user perspective, I very much dislike a tool or library reaching out to a server each time it is invoked, for activity unrelated to anything I explicitly requested or wanted. Please consider making this opt-in or at least opt-out.

I believe there is a possibility to opt-out via env variable (NO_ET). Unfortunately, due to necessity to provide funding agencies with proof of user base, we did agree (during last EAB meeting) that opt-in would be our way to go.

@kyleam
Copy link
Contributor

kyleam commented Dec 5, 2019

@satra

but your apps are local and they do tell you updates are available.

[ Sorry, I ignored the smartphone part of your reply since I don't use one. :/ ]

Even if you assume that your users are comfortable with their smartphones behaving this way (most of them probably are), that doesn't mean that they expect or prefer their, say, Debian systems to behave that way. And given that reproman isn't being used on smartphones, the latter is much more relevant.

i don't necessarily see that as concern if we ensure that people know that the app (even if it's a python tool on the command line) will communicate.

I agree with that. But as this PR stands (it's of course still work-in-progress) it doesn't look like we do anything to ensure that users know this or advertise how they can disable it.

@kyleam
Copy link
Contributor

kyleam commented Dec 5, 2019

@yarikoptic

I believe there is a possibility to opt-out via env variable (NO_ET).

Thanks.

yarikoptic added a commit to dbic/heudiconv that referenced this pull request Dec 24, 2019
Eventually some generalization of ReproNim/reproman#493
should become part of etelemetry, and thus avoiding redoing it here. Thus fix is
minimal

Closes nipy#405
pvelasco pushed a commit to cbinyu/heudiconv that referenced this pull request Feb 20, 2020
Eventually some generalization of ReproNim/reproman#493
should become part of etelemetry, and thus avoiding redoing it here. Thus fix is
minimal

Closes nipy#405
@yarikoptic
Copy link
Member Author

superseded with smaller #564

@yarikoptic yarikoptic closed this Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants