gocd-contrib / gomatic

A Python API for configuring GoCD
https://pypi.python.org/pypi/gomatic
MIT License
142 stars 59 forks source link

SSL and basic auth support #10

Closed macdiesel closed 7 years ago

macdiesel commented 8 years ago

Any interest in these changes?

Drakekin commented 8 years ago

Hello,

Thank you for your PR. I've got a few concerns though:

  1. The show_passwords arg is redundant, anyone who has access to the object already has the password, and the password shouldn't be in the __repr__ output anyway.
  2. Passing in a username and password should be optional, and should default to None to keep the signature the same.
  3. Can you please remove the mixing of - and _ in the CLI args?
macdiesel commented 8 years ago

@Drakekin updated, PTAL.

Drakekin commented 8 years ago

Have you run the integration tests?

It would be some additional work, but it would be really useful if you could expand the integration tests to also test this.

macdiesel commented 8 years ago

I can certainly run the integration suite to spot any errors. I got a 1 day spike to look at this. TBH I would rather spend some time adding in support to iterate and allow reverse engineering of all pipelines on a server, not just the one specified by name. If I can get a few features added here and we decide to use this it's likely you could see more PRs coming your way.

I would really like to add versioning/state output that could be used to track changes to the server over time similar to what terraform does. Is there a feature/product roadmap?

hanej commented 8 years ago

We are very interested in this commit. Thank you for filling a huge void in the Go API.

dudadornelles commented 8 years ago

+1 on this one, waiting for the merge!

compwron commented 8 years ago

+1

compwron commented 8 years ago

I forked and merged this PR. I do not intend to maintain this fork but it has what my team needs for now. https://github.com/compwron/gomatic

hanej commented 8 years ago

Can this be merged in?

Drakekin commented 8 years ago

Without tests, or any assurance that the existing integration tests have been run, I can't merge this.

I'm afraid there isn't a roadmap, or any plans to add any new features internally at present. I'm not opposed to people providing new features or functionality, but there are few resources right now on our side to support that.

I apologise for the delay in addressing this, but we've been quite busy.

macdiesel commented 8 years ago

I have a ticket in our backlog to come back and do the integration testing work. Hopefully we'll get it in the one of the next 2 sprints.

moritzheiber commented 8 years ago

Am I missing something obvious here? Travis says all tests are green .. ? :smiley:

macdiesel commented 8 years ago

@Drakekin I managed to get some time in to our current sprint to look at adding the integration testing.

I have created a new branch here: https://github.com/macdiesel/gomatic/tree/macdiesel/test-integration configuring ssl and tls for go-server in java in a way that python 2.7.10 plays well with has been problematic. Different versions of go-server need different go-server configuration file formats, some older versions will not start while newer versions start just fine. I'm moving this ticket to blocked for now as I have used about 5 points of time on this and do not have a satisfactory solution to the issue.

To be honest, I don't think that the time already spent to add the integration testing for the ssl/auth stuff is really worth the effort. I do think that this is patch that the community could really use and would urge you to merge it as it stands.

macdiesel commented 8 years ago

Reasons why I think this PR is awesome:

@doctoryes what else am I missing?

hanej commented 8 years ago

We're using this PR in production as well. Without it we couldn't use Gomatic at all since it's hooked into AD.

moritzheiber commented 8 years ago

Same here, we're using this PR in production since otherwise we wouldn't be able to run it against our setup at all

doctoryes commented 8 years ago

+1 on this PR - it works fine for us on GoCD 16.x and we couldn't use gomatic securely without it either.

clintonb commented 7 years ago

@ivanmoore @hilverd @Drakekin if the merge-conflict is resolved, can we merge this, please? It would be ideal if we did not have to resort to a fork to make use of gomatic.

moritzheiber commented 7 years ago

Honestly, at this point one might want to consider forking this project and turning it into an independent effort (maybe under a new name).

It's pretty clear Springer has abandoned it.

hilverd-springer commented 7 years ago

@clintonb @moritzheiber I am no longer involved in this project (since over a year ago) and neither is @ivanmoore. Forking it like @moritzheiber suggests seems like the best option, don't you agree @Drakekin?

moritzheiber commented 7 years ago

Can I take that as an official go ahead from Springer? I don't want this turn into a "hostile" fork, I'd rather just have the project live on in good spirits elsewhere.

clintonb commented 7 years ago

I, too, want to see this project continue. I cannot commit edX to maintaining a fork, but we are (as evident by this pull request) willing to contribute.

At a minimum, if Springer is no longer committing to maintain this project, the README should be updated with that information so potential maintainers can step forward or end-users can make fully-informed decisions in deciding if they want to use this library.

hilverd-springer commented 7 years ago

@moritzheiber I can't make that decision, but I'm sure the current maintainers (@Drakekin and others) will reply to this thread next week.

clintonb commented 7 years ago

Since the conversation has deviated from the SSL issue resolved by this pull request, I have opened a new issue to track the conversation around the state of this project. See https://github.com/SpringerSBM/gomatic/issues/17.

hilverd-springer commented 7 years ago

Thanks @clintonb. Anyone who is interested in taking over as maintainer please reply to #17.

moritzheiber commented 7 years ago

@macdiesel Can we move this along (finally)? There is a single conflict, I'd be happy to help!

macdiesel commented 7 years ago

@moritzheiber done!