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

Updated web_hook_controller.rb to include gitlab@ urls #4211

Merged
merged 1 commit into from Jan 25, 2018

Conversation

DavidWylie
Copy link
Contributor

Many Gitlab installations use gitlab@ as a git user. this was once a default user for gitlab installs.
Due to this the urls for ssh connections from gitlab can include this user.
Changing the user is a non trivial event.

This adds gitlab to this list so that it can be used in gocd.

@gocd-cla-bot
Copy link

gocd-cla-bot commented Jan 22, 2018

CLA assistant check
All committers have signed the CLA.

@ketan
Copy link
Member

ketan commented Jan 22, 2018

Since this code is specific to gitlab, it should ideally belong to git_lab_controller.rb and not the superclass web_hook_controller.rb — as this will affect other webhook implementations. Also, please update the corresponding spec file

@DavidWylie
Copy link
Contributor Author

@ketan Thanks for the feedback. Ive copied the function to the gitlab webhook controller and added the changes needed and added it to the spec tests.

@varshavaradarajan
Copy link
Member

@ketan
Copy link
Member

ketan commented Jan 25, 2018

I made a small tweak to your PR to extract the list of URLs into a method that can be overridden in each webhook subclass.

Many Gitlab installations use gitlab@ as a git user.  this was once a default user for gitlab installs.
Due to this the urls for ssh connections from gitlab can include this user.  Changing the user is a non trivial event.
@DavidWylie
Copy link
Contributor Author

Thanks.

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.

None yet

4 participants