Yeah, agree. So I guess we need to ask the password at the beginning if a user choices ssh method.
Ok, got it! Let’s finish this one and then continue with a new repo.
We should not. It’s only needed if a user doesn’t run ssh caching, so most likely this is not needed.
I was just indicating that we shouldn’t rely on ssh as passwordless method, so at this moment the script should abort unless a user has a github token - does it make sense?
Ok, got it! Let’s finish this one and then continue with a new repo.
Ah, ok, I was just about to remove it from the fastai repo. It’s no problem I will wait till this is done and then backport it to git-tools and then remove it from fastai.
Oh, got it. Sorry, I’ve incorrectly interpreted your note 
Yes, makes sense. Will commit a fix soon.
Actually, if that would be more convenient, I can finalize this PR, and then open a new one with the very same content in the git-tools repo, if you would like. So then we can remove these scripts from the main repo.
Whatever is easier for you, @devforfu. Either way works for me.
I was just going to copy it over when your PR was merged.
1 Like
@stas Ok, now only the token authentication is available. If a token is not provided, the script aborts. Therefore, the only way to run the script is to use https method with GITHUB_TOKEN environment variable. How do you think, it is a valid approach?
well, it cripples the script, but yes, that’s the only sure way if we can’t handle password prompts.
I guess the only other workaround we can add to support ssh w/o password prompts, is to do the following at the start of the script if ssh is chosen:
$ ssh -T git@github.com
Hi stas00! You've successfully authenticated, but GitHub does not provide shell access.
And if the password prompt appears or not the above is returned we abort.
Note that the above command is a failure return-status wise. It’s the message that matters.
But still so that this is not a drag for you, make a cut at say pre-workaround I suggested (supporting only https+token), commit, and let’s move on. And then we will look for someone else with time to complete this project.
1 Like
@stas Yes, I’ve already added the commit that enforces token and disables ssh for now. It is in my fork.
Great, thank you - let’s merge it then!
Since there is only one way now and you had it tested I trust your changes are good - I don’t currently have a setup to test on - so if there will be any issues we will deal with them as they come.
You added great instructions!
1 Like
Not a problem! =) Yes, I think it should work fine with the basic case we have now.
P.S.: Didn’t even know about getpass. Python’s standard library is a great source of useful tools. Though probably it is not that surprising taking into account the fact that Python is very often used as scripting and automating language.
1 Like
So, just to make sure we are not miscommunicating here - I’m ready to merge your PR, whenever you feel ready to submit it, @devforfu. Thanks.
@stas Yes, here it is. I think it should be in sync with the latest master. Also, meanwhile I’ll test it a couple more times just to make sure that it handles at least these limited cases we have.
Please let me know if I should submit this PR to some other repo.
Please let me know if I should submit this PR PR to some other repo.
Yes, if you’d like to - that’s why I suggested for you to submit it to https://github.com/fastai/git-tools so that the credits will get assigned to you, since otherwise they would be lost in copying.
@stas Ah, ok, understood! Then I’ll create a PR here also. One more opportunity to test the code: submit a PR with the help of the tool that is the subject of that PR
1 Like
Ok, here is a copy of the PR for the new repo:
And, also I realized that I’ve accidentally introduced error into the original PR in the final commit. Here is a fix. (A good illustration of how “simple and obvious” change without backtesting could prevent the code from working, I would say).
In the new PR, the mentioned bug is also fixed so it should work smoothly.
1 Like