Automatic hook registration #2

mattmoyer · 2017-10-06T19:42:37Z

I did a little bit of cleanup, but the meat of this is in 185d301 which sets up a background thread that automatically sets up webhooks and branch protection for all DCO repositories in our organizations.

The new README section explains how it works.

jbeda

Looks really good. Mostly nits.

Would be good to have a TODO or issues filed for a few things. One is graceful shutdown. Another is making sure we have timeouts on everything so that we don't hang around for everything. Getting rid of all of the context.TODO() would get us there.

jbeda · 2017-10-07T16:51:54Z

README.md


 ## Running
 There are two environment variables that need to be set when running:

 * `SHARED_SECRET`: Set this to a random value that you supply as the "secret" when configuring the webhook.
-* `GITHUB_TOKEN`: Set this to an personal access token for a github user that has access to the repo in question.  The webhook doesn't include details of the commits so we have to fetch them.  Unforutnately this requires full read/write `repo` access scope even though we are just reading.  Create one of these at https://github.com/settings/tokens.
+* `GITHUB_TOKEN`: Set this to a personal access token for a github user that has access to the repo in question.  The webhook doesn't include details of the commits so we have to fetch them.  Unfortunately this requires full read/write `repo` access scope even though we are just reading.  Create one of these at https://github.com/settings/tokens.


The "just reading" comment is no longer accurate as we are writing hooks, right?

jbeda · 2017-10-07T16:56:48Z

cmd/sign-off-checker/main.go

-	default:
-		log.Printf("Unhandled hook type: %v", hooktype)
+	publicWebhookURL, _ := os.LookupEnv("PUBLIC_WEBHOOK_URL")
+	if publicWebhookURL == "" && len(autoRegisterOrgs) > 0 {


After these checks pass might be good to output the config stating that autoregistration is active and the list of orgs and the webhook.

jbeda · 2017-10-07T17:01:50Z

cmd/sign-off-checker/main.go


-func loggingMiddleware(handler http.Handler) http.Handler {
+func loggingMiddleware(log *log.Logger, handler http.Handler) http.Handler {


NB: At some point ~soon we'll want to standardize on the error reporting and logging stuff that @ncdc has been working on. Probably doable after this though.

+1, I'll do a followup PR to swap this over to logrus and add in graceful shutdown and liveness checks using the other libraries I've been working on.

jbeda · 2017-10-07T17:02:43Z

cmd/sign-off-checker/main.go

-			break
-		}
-	}
+	dryRunString, _ := os.LookupEnv("DRY_RUN")


Not documented in README. Also makes me think that we should start moving to command line flags for this stuff. The only reason other params were env variables is that it is bad form to put secrets on the command line.

Sounds good. I'll pull in spf13/cobra + spf13/viper and update the command to use that.

jbeda · 2017-10-07T17:04:20Z

pkg/register/hook.go

+		}
+		for _, hook := range hooks {
+			// if the hook with our expected URL already exists, we're done
+			if hook.Config["url"] == url {


Hmm -- the secret may be wrong. But I'm guessing that this doesn't return the secret. At some point we may want to have a one shot mode to force re-registration in the event we rotate the secret. That might be a good roadmap/TODO thing.

Yeah, you can't really validate that the secret matches. You just have to overwrite it if you want to make sure it's correct. I'll file an issue for the force resync command.

jbeda · 2017-10-07T17:04:42Z

pkg/register/hook.go

+}
+
+func addSignOffHook(gh *github.Client, org string, repo *github.Repository, url string, secret string) error {
+	panic(nil)


Left over debugging?

jbeda · 2017-10-07T17:07:43Z

pkg/register/protection.go

+				EnforceAdmins: true,
+				RequiredStatusChecks: &github.RequiredStatusChecks{
+					Strict:   false,
+					Contexts: []string{},


This if is an early exit so we don't fall through to the code below where we automatically add the context. It would get done on the next run probably but would be good to get it in one shot.

Thanks. I refactored this a bit to split apart hasBranchProtection and addBranchProtection and this got dropped. I fixed it so it converges in a single step.

jbeda · 2017-10-07T17:09:11Z

pkg/register/register.go

+			return nil, fmt.Errorf("Error getting repositories for organization %q: %v", org, err)
+		}
+		result = append(result, repos...)
+		if resp.NextPage == 0 {


NB: the paging stuff is a total pain. Would be nice for the library to abstract that.

What might be really nice is something like client-go SharedInformer that could update repository state based on webhook events but also periodically resync.

Signed-off-by: Matt Moyer <[email protected]>

Adds a background thread that automatically configures webhooks and branch protection settings for all DCO repositories in a whitelist of organizations. Signed-off-by: Matt Moyer <[email protected]>

Signed-off-by: Matt Moyer <[email protected]>

mattmoyer · 2017-10-10T19:07:10Z

@jbeda updated, should be ready for a fresh review when you have a minute (thanks again!).

Signed-off-by: Matt Moyer <[email protected]>

ncdc · 2017-10-10T19:13:05Z

cmd/sign-off-checker/main.go

-	})
-}
+	// --listen / $LISTEN
+	rootCmd.Flags().String(


IIRC this returns the flag, so you might be able to pass that to the BindPFlag call on the next line.

It returns a *string that holds the flag's value and I can't see an easy way to define the flag and bind it into viper in a single shot. I am not a fan of the stringly typed interfaces here.

I thought about defining some constants for the viper config keys like const sharedSecretKey = "sharedSecret" so at least I've got a shot at catching the bug if I typo it to "shareSecret" somewhere.

I took a shot at this in 481c419.

ncdc · 2017-10-10T19:13:57Z

cmd/sign-off-checker/main.go

+	viper.BindEnv("listenAddress", "LISTEN")
+
+	// --autoregister / $AUTOREGISTER_ORGANIZATIONS
+	rootCmd.Flags().StringSlice(


I've never liked how you have to repeat the flag to get multiple values... For Ark we created our own array/slice flag type that supports comma-separated values. Not sure how much that matters here.

It's not documented well but Cobra also supports comma separated style. This also works with --autoregister Org1,Org2,Org3

Hmm, it didn't the last time we tried it.

It looks like only StringSlice splits on commas and StringArray doesn't (spf13/pflag#90)?

ncdc · 2017-10-10T19:17:54Z

cmd/sign-off-checker/main.go

+	return serveWebhook(gh)
+}
+
+func autoregister(gh *github.Client) {


Do you want to be able to stop this (stop channel or context)?

I'm hoping to add graceful shutdown (using this code: https://github.com/heptio/shutdown/pull/1) in a followup PR.

Signed-off-by: Matt Moyer <[email protected]>

This should make it a little harder to typo the viper key values and end up with nil somewhere we shouldn't. Signed-off-by: Matt Moyer <[email protected]>

davecheney · 2017-10-11T00:59:27Z

pkg/register/hook.go

+func hasSignOffHook(gh *github.Client, org string, repo *github.Repository, url string) (bool, error) {
+	opt := &github.ListOptions{PerPage: 10}
+	for {
+		hooks, resp, err := gh.Repositories.ListHooks(context.TODO(), org, repo.GetName(), opt)


I think you should use context.Background() here, context.TODO() blows up when it's used ... which tells me that it isn't actually used (insert rant about context here)

davecheney · 2017-10-11T00:59:51Z

pkg/register/hook.go

+			// 404 just means there are no hooks for this repo
+			return false, nil
+		}
+		if err != nil {


please handle the error first, then the other values after that.

davecheney · 2017-10-11T01:00:51Z

pkg/register/hook.go

+	if err != nil {
+		return fmt.Errorf("Error registering webhook: %v", err)
+	}
+	return err


return nil?

davecheney · 2017-10-11T01:01:18Z

pkg/register/protection.go

+
+func hasBranchProtection(gh *github.Client, org string, repo *github.Repository) (bool, error) {
+	contexts, resp, err := gh.Repositories.ListRequiredStatusChecksContexts(
+		context.TODO(),


s/TODO()/Background here and everywhere else

davecheney · 2017-10-11T01:03:58Z

pkg/register/protection.go

+// The GH API forces us to get+modify+set which means this could race with other
+// concurrent modifications.
+func addBranchProtection(gh *github.Client, org string, repo *github.Repository) error {
+	existing, resp, err := gh.Repositories.GetBranchProtection(context.TODO(), org, repo.GetName(), repo.GetDefaultBranch())


oh, reading the code for context TODO and Background() do the same thing internally ... and the promises of sufficiently advanced static analysis probably don't apply outside google3. Still, better to use context.Background()

davecheney · 2017-10-11T01:04:49Z

pkg/register/protection.go

+// concurrent modifications.
+func addBranchProtection(gh *github.Client, org string, repo *github.Repository) error {
+	existing, resp, err := gh.Repositories.GetBranchProtection(context.TODO(), org, repo.GetName(), repo.GetDefaultBranch())
+	if err != nil && resp != nil && resp.StatusCode != 404 {


if err was not nil, then you should probably not assume anything about the state of resp, it's not safe to call.

davecheney · 2017-10-11T01:05:11Z

pkg/register/protection.go

+// concurrent modifications.
+func addBranchProtection(gh *github.Client, org string, repo *github.Repository) error {
+	existing, resp, err := gh.Repositories.GetBranchProtection(context.TODO(), org, repo.GetName(), repo.GetDefaultBranch())
+	if err != nil && resp != nil && resp.StatusCode != 404 {


Is resp a http.Response object? Does resp.Body need to be closed here and everywhere?

davecheney · 2017-10-11T01:07:31Z

pkg/register/register.go

+
+// Package register implements automatic registration of webhooks and branch
+// protections on all DCO repositories in a set of organizations.
+package register


suggestion: a package's name should be a one word description of what it provides. webhooks might be a better name than register, the verb.

davecheney · 2017-10-11T01:08:10Z

pkg/register/register.go

+// Register walks the provided organization, finds repositories that use the
+// Developer Certificate of Origin (in CONTRIBUTING.md), and registers the
+// sign-off-checker webhook and required commit statuses in each repository.
+func Register(log *log.Logger, gh *github.Client, dryRun bool, organizations []string, webhookURL string, webhookSecret string) error {


suggestion: RegisterWebhooks might be a better name.

mattmoyer requested review from jbeda and chuckha October 6, 2017 19:42

jbeda suggested changes Oct 7, 2017

View reviewed changes

mattmoyer added 11 commits October 10, 2017 14:04

Handle Ping events with no error message.

2044d23

Signed-off-by: Matt Moyer <[email protected]>

Clean up regexp init a bit.

424d2b0

Signed-off-by: Matt Moyer <[email protected]>

Use the github.String() helper instead of our own s().

7ca2324

Signed-off-by: Matt Moyer <[email protected]>

Pull this "context" name into a constant.

4ed3b0b

Signed-off-by: Matt Moyer <[email protected]>

Add automatic registration loop.

e2385cc

Adds a background thread that automatically configures webhooks and branch protection settings for all DCO repositories in a whitelist of organizations. Signed-off-by: Matt Moyer <[email protected]>

Move to heptiolabs.

a11d10c

Signed-off-by: Matt Moyer <[email protected]>

Fix README note about needing full repo access.

1b6300b

Signed-off-by: Matt Moyer <[email protected]>

Add some logs about whether autoregistration is enabled.

9b8ec3f

Signed-off-by: Matt Moyer <[email protected]>

Drop debugging panic().

195ee15

Signed-off-by: Matt Moyer <[email protected]>

Add the required context when configuring a hook for the first time.

d3f211b

Signed-off-by: Matt Moyer <[email protected]>

Update to use CLI flags (spf13/cobra).

7e6eabf

Signed-off-by: Matt Moyer <[email protected]>

mattmoyer force-pushed the autoregistration branch from 2ecd318 to 7e6eabf Compare October 10, 2017 19:05

mattmoyer added 2 commits October 10, 2017 14:11

Convert from godeps to dep and update dependencies.

c7bcc35

Signed-off-by: Matt Moyer <[email protected]>

Add CoC.

ec6c534

Signed-off-by: Matt Moyer <[email protected]>

ncdc reviewed Oct 10, 2017

View reviewed changes

mattmoyer added 2 commits October 10, 2017 15:20

Fix case of log message to be consistent.

17e7d47

Signed-off-by: Matt Moyer <[email protected]>

Pull flag names into constants, bind them all at once.

481c419

This should make it a little harder to typo the viper key values and end up with nil somewhere we shouldn't. Signed-off-by: Matt Moyer <[email protected]>

mattmoyer removed the request for review from chuckha October 10, 2017 20:24

mattmoyer added the enhancement label Oct 10, 2017

davecheney reviewed Oct 11, 2017

View reviewed changes


		func loggingMiddleware(handler http.Handler) http.Handler {
		func loggingMiddleware(log *log.Logger, handler http.Handler) http.Handler {

Automatic hook registration #2

Are you sure you want to change the base?

Automatic hook registration #2

Conversation

mattmoyer commented Oct 6, 2017 • edited Loading

jbeda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoyer commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoyer commented Oct 6, 2017 •

edited

Loading