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

Fix mulekick dependency removal #585

Merged
merged 7 commits into from
Jul 13, 2019

Conversation

megakoresh
Copy link
Contributor

@megakoresh megakoresh commented Jul 9, 2019

Removal of mulekick dependency has broken the whole project. (that time I said it "builds" fine for me? Yeah, it didn't, I confused "compile" with "build"). This should fix it by changing the code to use gorilla/mux router directly. Mulekick has been effectively removed.

Two functions that remained have been moved to util package. router.go contains most of the changes. Looks a bit uglier in places, but hopefully more robust and transparent now.

With my node.js background I tried to make mux behave like Koa first, which is not a bright idea of course. Still - could be useful, so I kept it in history.

@megakoresh
Copy link
Contributor Author

There is some issue with CI setup, specifically with go-task, but I don't know how to resolve that. Anyway I have built and tested it also manually by just clicking around in the UI, nothing appears to be broken, at least based on what I have seen. And at any rate it builds fine, which is better than current state of upstream. Maybe @matejkramny can take a look and merge this?

@matejkramny
Copy link
Contributor

@strangeman any comments? Looks good to me.

One thing i'm noticing (unrelated to the PR), the gorilla/context package is deprecated in favour of r.Context. In other projects it was causing me severe memory leaks.

@matejkramny matejkramny mentioned this pull request Jul 11, 2019
@megakoresh
Copy link
Contributor Author

@strangeman any comments? Looks good to me.

I guess the guy has left somewhere. Can you merge this? As much potential as I see with this project, it doesn't give a strong impression with stagnation like this. Besides - he merged the PR that removed mulekick initially, which broke the upstream completely, it can't get worse if you merge a fix to that.

Frankly to get more activity on the project, requirements to merge stuff need to be loosen and speed of merging and closing issues increased. Not enough people are active on, or using this software for that to be a risk at this stage anyway. A bigger risk is the seeming stagnation of the community, since despite all the potential this project has as an alternative to the (very faulty) AWX/Tower, it won't be a serious contender to it, if it doesn't get more popular and gain a more active community.

@matejkramny
Copy link
Contributor

fair point @megakoresh. I'll take that into consideration.

@matejkramny matejkramny merged commit 3db758c into semaphoreui:develop Jul 13, 2019
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.

2 participants