Sunset svn git access #205
Conversation
|
@jedwards4b - thank you very much for your quick work to resolve these issues! I see that you removed some system tests of the svn protocol. I'm guessing you did that because they were set up to use the svn protocol to access a GitHub repo. I haven't looked closely at the tests, but I'm wondering if we can switch them to access an actual svn repo so that we can keep these tests in place. For example, cgd still maintains an svn server (which I'm embarrassed to admit that I still use for some personal repos that I've never bothered to switch over to git)... we could set up a little test repository there. If this makes sense to you, I could take a stab at it. |
|
I looked at that test and thought that it was specific to svn access to git, I assumed that there were other tests for svn but to be honest I didn't look carefully. I was really more concerned about the submodule testing which I finally managed to fix. |
|
Okay, let me see if I can get this working with an actual svn repo. I'm going to try using a little local test repo that we can access with the |
manic/repository_git.py
Outdated
| file_protocol = "" | ||
| if "test/tmp/test_submodule" in dirname: |
There was a problem hiding this comment.
Having this in the main code feels like a security hole waiting to cause trouble.
mkdir -p test/temp/test_submodule && cd test/temp/submodule
git clone . . .
At this point, can't I call into this method with a dirname that matches this 'magic' pattern?
Instead of hardcoding this magic pattern into the mainline manic code, is it possible to set this flag inside the GitHub CI so that it is active when the test of this method runs?
There was a problem hiding this comment.
I am happy to consider your alternate proposal. I spent an inordinate amount of time to come up with this solution, but I certainly don't want to introduce any security issues. If you feel that the security risk is greater than the risk of not testing this then we should go back to just removing these tests. It's not just the github CI that fails without this change - the tests fail when you run make stest on your local system as well.
There was a problem hiding this comment.
I'm happy to consider other ways to figure out if I am in a test context and should thus apply the file_protocol while in
_git_update_submodules.
Would it be preferable to pass an optional variable testing=True all the way down the stack? It's 6 or 7 layers deep.
There was a problem hiding this comment.
@gold2718 How about now?
There was a problem hiding this comment.
One other possible idea: instead of making file_protocol a local variable, make it a function (or a method of the GitRepository class). Its implementation in production code will be to just return an empty string. In unit tests, we could use mocking to replace it with a function that returns this special string.
Oh, I see you just pushed a new change... I'm okay with that solution, too.
We can no longer use a GitHub repo for svn testing because GitHub is disabling access via the svn protocol.
|
@jedwards4b - see #206 for a fix to the svn-based sys tests. If that looks reasonable to you, can you please back out most of your changes in test_sys_checkout.py – other than the last two lines of changes, which change the git submodule commands – and then merge my changes in #206 into this branch (we can either merge them to main and you can update this branch to main, or you could merge my branch directly into this branch). |
|
@gold2718 have you looked at this new solution? Do you still have concerns? |
Sorry, been in meetings. Will grab a look now |
| # submodules from file doesn't work without overriding the protocol, this is done | ||
| # for testing submodule support but should not be done in practice | ||
| file_protocol = "" | ||
| if 'unittest' in sys.modules.keys(): |
There was a problem hiding this comment.
Nice! Not perfect but more than enough of a cross check for this sort of application.
Remove support for svn access to github, fix submodule tests which are broken due to a github security issue. Fix svn tests by creating a local svn repo for testing - thanks @billsacks
User interface changes?: Yes
github no longer supports svn access
Fixes: #202
Fixes: #203
Testing:
test removed: NONE
unit tests: all pass
system tests: all pass
manual testing: