Port fastai-make-pr-branch from bash to python


(Ilia) #41

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.


(Stas Bekman) #42

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.


(Ilia) #43

Oh, got it. Sorry, I’ve incorrectly interpreted your note :smile:

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.


(Stas Bekman) #44

Whatever is easier for you, @devforfu. Either way works for me.

I was just going to copy it over when your PR was merged.


(Ilia) #45

@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?


(Stas Bekman) #46

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.


(Ilia) #47

@stas Yes, I’ve already added the commit that enforces token and disables ssh for now. It is in my fork.


(Stas Bekman) #48

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!


(Stas Bekman) #49

Some notes for part 2: