Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Privileged builds support #131

Merged
merged 2 commits into from Mar 19, 2014

Conversation

Projects
None yet
2 participants
Contributor

vito commented Feb 26, 2014

With this PR an admin can enable a repository to run its builds privileged. Builds for pull requests will not run privileged. This is exposed to the build via a $DRONE_PRIVILEGED environment variable, so that tests that require privileged can be disabled when running against a pull request.

This PR also includes some plumbing for writing tests against a fake docker client and fake Dockerfile writer. Currently it's only used for the build tests which were added to test only the privileged functionality. In the future they can be reused more generally.

Also adds a -privileged flag to the drone CLI.

fixes #85

Owner

bradrydzewski commented Feb 26, 2014

Hey do you think we can take this approach to testing, as opposed to interfaces?
https://willnorris.com/2013/08/testing-in-go-github

Adding interfaces and mocks to all the different layers (database, github, bitbucket, etc) ended up creating a lot of abstraction and boilerplate mocking. We did this originally, and moved away from this approach.

I realize it may require a bit of refactoring (and I really hate having to ask people to make changes) so I'm happy to pitch in if you want to assign me some tasks to help

Owner

bradrydzewski commented Feb 26, 2014

by the way, I'm trying to get better about being on IRC, but you can always IM me on gchat brad.rydzewski@gmail.com

Contributor

vito commented Feb 27, 2014

Totally OK with changing pull requests based on your input. Having read that post, what I think you're saying is you'd rather fake out (or not) the Docker server than the client?

In my experience faking out the client gives you much more fine-grained control, especially when simulating error cases. For example, a few clients I've written have tests that simulate an interrupted connection by injecting a client that has a pre-baked net.Conn that will serve a payload, break, and then serve another payload. It's often very sensitive code, and you either get it right or your app deadlocks. This is very hard to reliably do by faking out the server.

And in those cases, even when faking out the server I'd probably inject the client anyway. I find it much easier to find the flow of config/user inputs down into the relevant objects. Having interfaces just makes it very testable; I think it's one of Go's biggest strengths. There is an initial hurdle as you watch your constructor argument length grow, but I've found that making your dependencies explicit is not a bad thing, and pushes you towards having smaller, self-contained, more composeable objects that you can keep wholly in your head.

For example, one fake object I've written 1,000 times is a Command Runner, which will run a given exec.Cmd. It seems silly at first: all you normally do is call .Run on the command. But by injecting a fake, you can make assertions on not just whether they were invoked, but the order in which they were executed, the environment they ran with, etc. And by making it an interface, I was able to extend my entire application to running against a remote VM by writing a RemoteCommandRunner that just ran its commands over SSH. The point being that sometimes even the silliest of interfaces saves you a lot of hassle down the road.

That being said, in my tests I'll also pretty often spin up a real live instance of the service my project uses. I think spinning up Docker is pretty expensive though; not in terms of performance, but in portability.

Anyway, that's a lot of words, but here are some real life projects that follow this approach, if you want to see the extent of it:

Small:
https://github.com/cloudfoundry-incubator/linux-smelter/blob/master/smelter/smelter_test.go

Large:
https://github.com/pivotal-cf-experimental/garden/blob/master/linux_backend/linux_container_test.go#L443

Owner

bradrydzewski commented Feb 27, 2014

I've definitely gone back and forth on this. The first version of Drone that we built used interfaces heavily for testing purposes. I discussed here:

https://groups.google.com/d/msg/golang-nuts/9i01tuVo-1E/f_VWPsIfRFYJ

In this version of Drone we took a very different approach, creating a new database and populating with a fixed dataset for each unit test. I ended up really liking this approach:

https://github.com/drone/drone/blob/master/pkg/database/testing/testing.go#L40
https://github.com/drone/drone/blob/master/pkg/handler/testing/users_test.go#L18

It seems like the Go community is split into two camps. Some using interfaces and mocking. Others using approaches like Will's for http servers, or this approach for command line:

https://github.com/globocom/commandmocker

I'm not sure exactly where I fall.

So here is what I propose:

  • You leave your PR as-is for now
  • I'll create a few unit tests using Will's proposed approach
  • We compare the two approaches and discuss the pros and cons
  • We ask a few other Go devs to join the discussion

I know this might seem like a lot of effort to make a decision, but this is a pretty important structural change. I'll try to make this as painless as possible :)

Owner

bradrydzewski commented Feb 27, 2014

as a side note, I completely agree that we should remove the global instances. I like the approach you took with the Docker client, Queue, etc.

Contributor

vito commented Feb 27, 2014

Sounds great. I'd be happy to check out other approaches.

Contributor

vito commented Mar 6, 2014

Any update on this?

Thanks

Owner

bradrydzewski commented Mar 7, 2014

hey, sorry for delayed response. I'm working on tests for Bitbucket integration:
https://github.com/drone/drone/blob/bitbucket/pkg/handler/bitbucket.go

I'll try to use this method:
https://willnorris.com/2013/08/testing-in-go-github

It should give us a comparison, and then we can pull in a few people to weigh in on pros and cons. I'll try to speed this up. Sorry I've been slow. Too many meetings!!!

Owner

bradrydzewski commented Mar 9, 2014

hey, quick question. thoughts on this:
https://github.com/gocraft/web

If we end up injecting services that we could do so via middleware using this library. I haven't had the chance to play around with it yet. Have you?

Owner

bradrydzewski commented Mar 10, 2014

I am making progress on this. Here is a snippet of code I'm working on locally:
https://gist.github.com/anonymous/9460601

It uses text fixtures instead of mocks. There is an interesting article here about the two:
http://martinfowler.com/articles/mocksArentStubs.html#FixtureSetup

Owner

bradrydzewski commented Mar 19, 2014

OK, ready to get some of this merged.

Can you revert changes to /pkg/build/** so that we can merge everything else, including changes to the website, queue and drone command line tool?

I added the Privileged field to the builder, logic to prevent building pull requests, and unit tests:
https://github.com/drone/drone/blob/master/pkg/build/build.go#L66
https://github.com/drone/drone/blob/master/pkg/build/build.go#L312

I've improved the test coverage of /pkg/build (65%), pkg/build/buildfile (100%), and pkg/build/dockerfile (100%). You can see an example test case for the build code here:
https://github.com/drone/drone/blob/master/pkg/build/build_test.go#L154

For now, I'd like to stick with test fixtures. I plan to switch to the go-dockerfile package which is not using interfaces and would not support injection. With fixtures, our unit tests will not need to be re-written. Same goes with GitHub and Gitlab libraries.

I would like to continue removing global variables and injecting instances, as you have done. The next logical step may very well be to replace implementations with interfaces, but let's tackle that separately.

Contributor

vito commented Mar 19, 2014

Awesome! I've stripped my PR down to 2 commits. One feature that's been dropped is notifying a build that it's not running privileged (the DRONE_PRIVILEGED env var). That can be a separate change later though.

Owner

bradrydzewski commented Mar 19, 2014

looks like there is a compile issue. missing Repo.Privileged?

Alex Suraci add --privileged to drone build command
Signed-off-by: Alex Suraci <asuraci@pivotallabs.com>
fc3715e
Contributor

vito commented Mar 19, 2014

woops. fixed!

@bradrydzewski bradrydzewski added a commit that referenced this pull request Mar 19, 2014

@bradrydzewski bradrydzewski Merge pull request #131 from vito/privileged-builds
Privileged builds support
b1cbc21

@bradrydzewski bradrydzewski merged commit b1cbc21 into drone:master Mar 19, 2014

1 check passed

default The build succeeded on drone.io
Details
Owner

bradrydzewski commented Mar 19, 2014

thanks! if you send a pr for the drone_privileged environment variable I'll get that merged as well.

we should also take a look at the logic for accessing a repository:
https://github.com/drone/drone/blob/master/pkg/handler/handler.go#L124
https://github.com/drone/drone/blob/master/pkg/handler/handler.go#L124

currently an admin cannot access a repository unless they are the owner or a team member. For your changes to be truly useful, we should probably allow an admin to visit any repository so they can set the privileged flag.

thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment