kubitron / redmine_git_hosting

A ChiliProject/Redmine plugin which makes configuring your own git hosting easy.
78 stars 15 forks source link

allow alternate port in gitServer parameter #2

Closed simonsd closed 12 years ago

simonsd commented 12 years ago

Allow the usage of an alternate port in the gitServer parameter. For now this is only allowed in the http url setting, but the gitServer param is the one being used to clone the gitolite-admin repo.

Make sure that when using any port besides 22, you prefix the gitServer url with 'git+ssh://', and replace the colon after the url with a '/' to separate the port from the repo specifier.

On a sidenote you also need to make sure you patch the git_url_ssh param in assets/javascripts/git_url_display.js:7 Same for the app/views/repositories/git_instructions.html.erb:3 And probably some others as well.

kubitron commented 12 years ago

Hello, @simonsd

Question: Why use the longer "git+ssh://" syntax instead of "ssh://" ? The former syntax doesn't seem to be regularly documented any more...?

simonsd commented 12 years ago

My bad, didn't know about that. Tried it out for a bit and it does indeed seem to provide the same functionality as the other one. In that case please use just the 'ssh://' prefix. And thanks for the heads up :)

kubitron commented 12 years ago

Ok. You can try the "testing" branch. Does this work for you?

Note that I didn't have any problem using the same clone syntax for with and without the port...

simonsd commented 12 years ago

Thanks for the fast work. Just tried it out a bit and it works perfectly. Only drawback I noticed was that the url on the introduction page is being shown as 'ssh://git@hostname:port/project1/project2/project3.git' Is suppose this is intentional? I understand what to do when looking at this, but I wonder wether the end users will feel the same way.

Since this obsoletes the stuff I wrote I'll close the pull request and throw away the code.

kubitron commented 12 years ago

Oops. My bad. That is a cut-and-paste error. Will fix later this morning when I get a chance.

kubitron commented 12 years ago

Hm... Oh... Do you mean on the green "access" box on the settings page? The opening pararaph sets up what project1, 2, and 3 are. Is that not clear? Note that (1) this information is only for administrators configuring the plugin (play with some of the new options, like the "hierarchical" vs "flat" and you will see why it is there), (2) it has to be generic because it is not associated with any particular project.

What if the opening sentence had project 1, 2, and 3 highlighted. Would that help?

simonsd commented 12 years ago

no, the green box is perfectly clear. And it is indeed only visible to the administration when configuring the plugin, so it shouldn't be an issue anyways.

I mean on a projects repository page, when the repository is still empty. It has a placeholder where otherwise the source tree and commit log would be, explaining the steps to take in order to successfully push something to the repo for the first time.

simonsd commented 12 years ago

perhaps a bit unrelated, but still along the same lines: I get an error when adding a repo mirror with an alternate port specified. error goes like:

NameError (undefined local variable or method `mirror' for # RepositoryMirror:0x7f11e77b2578>):

Mirror without an alternate port work just fine. Should I open a new issue for this perhaps?

kubitron commented 12 years ago

Ok. My bad on the empty repo page. That really was a cut-and-paste bug (what I get for hacking while watching tennis).

Pull the next commit off testing and see if it fixes things. (Also, I decided to emphasize the project labels in the green box as well).

As for the weird error, I think that this is actually a bug in the mirror push failure case. I think I fixed it (I pushed it up to the testing branch). Try doing what you were doing again -- I believe you should get an error in the log and/or on screen. Of course, this begs the question of why you had an error.

Is your mirror URL in the right format for a port? (i.e. I think you need to say: ssh://user@server.com:port/repo.git).

simonsd commented 12 years ago

Awesome, thanks for the fixes. The urls on the git introduction page are fixed now, tried creating a few new repos and the urls always show up as they should. As for the mirror issues: they're persistent. However I seem to have found the problem. The plugin apparently doesn't handle mirror urls with a minus (-) in them correctly. The logs are now returning errors saying:

ArgumentError (negative argument):
  vendor/plugins/redmine_git_hosting/app/models/repository_mirror.rb:28:in `*'

And a traceback, but I suppose the most important stuff is in the first two lines. If you need more info just ask.

As for the mirror url being in the right format, I think so. I tested it with a few other repos without a dash in the url, and they all work flawlessly.

kubitron commented 12 years ago

Sigh. Now, that is the goofy error message handling code that I partially fixed, but that doesn't now handle long urls.

Can you tell me what your URL looks like? (I'm assuming that does not have a minus in it, right?)

kubitron commented 12 years ago

At any rate, pull the testing branch again and try again. I'm hoping that you will get an actual error in the log/on the screen rather than dead code.

simonsd commented 12 years ago

Awesome, that latest commit fixes my problem. I'm still a bit confused about the 'long urls' part. Do you mean the length of the urls actually is causing this?

kubitron commented 12 years ago

Um.. So it succeeds entirely? That doesn't make sense. The branch of the code I fixed only gets invoked if the push fails. Look at the log and tell me whether there is an error there.....

kubitron commented 12 years ago

Oh -- in answer to your question -- the code that was failing was on the path that gets invoked when a mirror push fails. It was purely for printing out error messages, and it was the formatting of the error messages that was broken and which failed with a long url. Nothing like non-robust code!

simonsd commented 12 years ago

erm .. ok .. you're right about it not making any sense. In the redmine ui it clearly says pushing succeeded, however when looking at the logs the pushing fails with 'permission denied'. Guess that's a good sign since the connection seems to be coming through .. Gonna try to get the authentication issues sorted out, I'll keep you up to date.

kubitron commented 12 years ago

Ok. Can I close this issue? I'll push the changes to my master branch.

simonsd commented 12 years ago

fixed auth issues, now the push to the mirror works. The redmine ui shows 'success' wether it succeeds or fails.

kubitron commented 12 years ago

Since I don't use mirrors, tell me what page/url this 'success' shows on...?

simonsd commented 12 years ago

it shows a template based on app/views/layouts/popup.erb

simonsd commented 12 years ago

Sure, issue may be closed. Thanks for all the help :)

kubitron commented 12 years ago

This fix now applied to master branch as commit: a472dbe.

kubitron commented 12 years ago

OK @simonsd -- When you get a chance, you could see if the code on my master branch fixes your problem with mirrors properly. Try both a failed and successful authentication. The status should be correctly reported now to the browser (as well as errors put into the log).

simonsd commented 12 years ago

Thanks for the heads up, will try it out in a bit.

simonsd commented 12 years ago

Ok, checked it out and it works as expected. Also kudos for letting failed jobs display indefinitely, while making successfully executed jobs' output dissapear after 5 secs. Good implementation, thanks a lot!

kubitron commented 12 years ago

Glad it works for you. The timing was there, just broken. I'll assume that everything works now for you.

Note that the testing branch was garbage collected (removed). Hopefully the master branch has everything you need now. Let me know if there are any other problems.

simonsd commented 12 years ago

Yeah, everything is just fine for me now. I noticed the testing branch was gone, and figured you squashed the commits and put them into the main branch like that. After removing the latest commits from testing from my local branch and pulling from master, all my problems are solved. I'll definitely let you if anything else pops up, and again, thanks a lot :)

kubitron commented 12 years ago

Glad to help. It will be interesting to have your users hammer on the plugin. Nothing like trial by fire.

Register new issues for new problems.

Later, --KUBI--