simplify gating logic

drone/server/server.go

@@ -400,7 +400,7 @@ func setupEvilGlobals(c *cli.Context, v store.Store) {
 	droneserver.Config.Services.Pubsub.Create(context.Background(), "topic/events")
 	droneserver.Config.Services.Registries = registry.New(v)
 	droneserver.Config.Services.Secrets = secrets.New(v)
-	droneserver.Config.Services.Senders = sender.New(v)
+	droneserver.Config.Services.Senders = sender.New(v, v)
 	if endpoint := c.String("registry-service"); endpoint != "" {
 		droneserver.Config.Services.Registries = registry.NewRemote(endpoint)
 	}

model/config.go

@@ -4,15 +4,14 @@ package model
 type ConfigStore interface {
 	ConfigLoad(int64) (*Config, error)
 	ConfigFind(*Repo, string) (*Config, error)
-	ConfigUpdate(*Config) error
-	ConfigInsert(*Config) error
+	ConfigFindApproved(*Config) (bool, error)
+	ConfigCreate(*Config) error
 }
 
 // Config represents a pipeline configuration.
 type Config struct {
-	ID       int64  `json:"-"        meddler:"config_id,pk"`
-	RepoID   int64  `json:"-"        meddler:"config_repo_id"`
-	Data     string `json:"data"     meddler:"config_data"`
-	Hash     string `json:"hash"     meddler:"config_hash"`
-	Approved bool   `json:"approved" meddler:"config_approved"`
+	ID     int64  `json:"-"    meddler:"config_id,pk"`
+	RepoID int64  `json:"-"    meddler:"config_repo_id"`
+	Data   string `json:"data" meddler:"config_data"`
+	Hash   string `json:"hash" meddler:"config_hash"`
 }

plugins/sender/builtin.go

@@ -6,15 +6,23 @@ import (
 
 type builtin struct {
 	store model.SenderStore
+	conf  model.ConfigStore
 }
 
 // New returns a new local gating service.
-func New(store model.SenderStore) model.SenderService {
-	return &builtin{store}
+func New(store model.SenderStore, conf model.ConfigStore) model.SenderService {
+	return &builtin{store, conf}
 }
 
 func (b *builtin) SenderAllowed(user *model.User, repo *model.Repo, build *model.Build, conf *model.Config) (bool, error) {
-	if !conf.Approved {
+	if build.Event == model.EventPull && build.Sender != user.Login {
+		// check to see if the configuration has already been used in an
+		// existing build. If yes it is considered approved.
+		if ok, _ := b.conf.ConfigFindApproved(conf); ok {
+			return true, nil
+		}
+		// else check to see if the configuration is sent from a user
+		// account that is a repositroy approver themselves.
 		sender, err := b.store.SenderFind(repo, build.Sender)
 		if err != nil || sender.Block {
 			return false, nil

server/build.go

@@ -180,10 +180,6 @@ func PostApproval(c *gin.Context) {
 		c.AbortWithError(404, err)
 		return
 	}
-	if !conf.Approved {
-		conf.Approved = true
-		Config.Storage.Config.ConfigUpdate(conf)
-	}
 
 	netrc, err := remote_.Netrc(user, repo)
 	if err != nil {
@@ -404,10 +400,6 @@ func PostBuild(c *gin.Context) {
 		c.AbortWithError(404, err)
 		return
 	}
-	if !conf.Approved {
-		conf.Approved = true
-		Config.Storage.Config.ConfigUpdate(conf)
-	}
 
 	netrc, err := remote_.Netrc(user, repo)
 	if err != nil {

server/hook.go

@@ -142,27 +142,17 @@ func PostHook(c *gin.Context) {
 	conf, err := Config.Storage.Config.ConfigFind(repo, sha)
 	if err != nil {
 		conf = &model.Config{
-			RepoID:   repo.ID,
-			Data:     string(confb),
-			Hash:     sha,
-			Approved: false,
+			RepoID: repo.ID,
+			Data:   string(confb),
+			Hash:   sha,
 		}
-		if user.Login == repo.Owner || build.Event != model.EventPull || repo.IsGated == false {
-			conf.Approved = true
-		}
-		err = Config.Storage.Config.ConfigInsert(conf)
+		err = Config.Storage.Config.ConfigCreate(conf)
 		if err != nil {
 			logrus.Errorf("failure to persist config for %s. %s", repo.FullName, err)
 			c.AbortWithError(500, err)
 			return
 		}
 	}
-	if !conf.Approved {
-		if user.Login == repo.Owner || build.Event != model.EventPull || repo.IsGated == false {
-			conf.Approved = true
-			Config.Storage.Config.ConfigUpdate(conf)
-		}
-	}
 	build.ConfigID = conf.ID
 
 	netrc, err := remote_.Netrc(user, repo)

store/datastore/config.go

@@ -1,13 +1,15 @@
 package datastore
 
 import (
+	gosql "database/sql"
+
 	"github.com/drone/drone/model"
 	"github.com/drone/drone/store/datastore/sql"
 	"github.com/russross/meddler"
 )
 
 func (db *datastore) ConfigLoad(id int64) (*model.Config, error) {
-	stmt := sql.Lookup(db.driver, "config-find-repo-id")
+	stmt := sql.Lookup(db.driver, "config-find-id")
 	conf := new(model.Config)
 	err := meddler.QueryRow(db, conf, stmt, id)
 	return conf, err
@@ -20,10 +22,18 @@ func (db *datastore) ConfigFind(repo *model.Repo, hash string) (*model.Config, e
 	return conf, err
 }
 
-func (db *datastore) ConfigUpdate(config *model.Config) error {
-	return meddler.Update(db, "config", config)
+func (db *datastore) ConfigFindApproved(config *model.Config) (bool, error) {
+	var dest int64
+	stmt := sql.Lookup(db.driver, "config-find-approved")
+	err := db.DB.QueryRow(stmt, config.RepoID, config.ID).Scan(&dest)
+	if err == gosql.ErrNoRows {
+		return false, nil
+	} else if err != nil {
+		return false, err
+	}
+	return true, nil
 }
 
-func (db *datastore) ConfigInsert(config *model.Config) error {
+func (db *datastore) ConfigCreate(config *model.Config) error {
 	return meddler.Insert(db, "config", config)
 }

store/datastore/config_test.go

@@ -18,12 +18,11 @@ func TestConfig(t *testing.T) {
 		hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26"
 	)
 
-	if err := s.ConfigInsert(
+	if err := s.ConfigCreate(
 		&model.Config{
-			RepoID:   2,
-			Data:     data,
-			Hash:     hash,
-			Approved: false,
+			RepoID: 2,
+			Data:   data,
+			Hash:   hash,
 		},
 	); err != nil {
 		t.Errorf("Unexpected error: insert config: %s", err)
@@ -47,60 +46,102 @@ func TestConfig(t *testing.T) {
 	if got, want := config.Hash, hash; got != want {
 		t.Errorf("Want config hash %s, got %s", want, got)
 	}
-	if got, want := config.Approved, false; got != want {
-		t.Errorf("Want config approved %v, got %v", want, got)
-	}
 
-	config.Approved = true
-	err = s.ConfigUpdate(config)
+	loaded, err := s.ConfigLoad(config.ID)
 	if err != nil {
-		t.Errorf("Want config updated, got error %q", err)
+		t.Errorf("Want config by id, got error %q", err)
 		return
 	}
+	if got, want := loaded.ID, config.ID; got != want {
+		t.Errorf("Want config by id %d, got %d", want, got)
+	}
+}
 
-	updated, err := s.ConfigFind(&model.Repo{ID: 2}, hash)
-	if err != nil {
-		t.Errorf("Want config find, got error %q", err)
+func TestConfigApproved(t *testing.T) {
+	s := newTest()
+	defer func() {
+		s.Exec("delete from config")
+		s.Exec("delete from builds")
+		s.Close()
+	}()
+
+	var (
+		data = "pipeline: [ { image: golang, commands: [ go build, go test ] } ]"
+		hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26"
+		conf = &model.Config{
+			RepoID: 1,
+			Data:   data,
+			Hash:   hash,
+		}
+	)
+
+	if err := s.ConfigCreate(conf); err != nil {
+		t.Errorf("Unexpected error: insert config: %s", err)
 		return
 	}
-	if got, want := updated.Approved, true; got != want {
-		t.Errorf("Want config approved updated %v, got %v", want, got)
+
+	s.CreateBuild(&model.Build{
+		RepoID:   1,
+		ConfigID: conf.ID,
+		Status:   model.StatusBlocked,
+		Commit:   "85f8c029b902ed9400bc600bac301a0aadb144ac",
+	})
+	s.CreateBuild(&model.Build{
+		RepoID:   1,
+		ConfigID: conf.ID,
+		Status:   model.StatusPending,
+		Commit:   "85f8c029b902ed9400bc600bac301a0aadb144ac",
+	})
+
+	if ok, _ := s.ConfigFindApproved(conf); ok == true {
+		t.Errorf("Want config not approved, when blocked or pending")
+		return
+	}
+
+	s.CreateBuild(&model.Build{
+		RepoID:   1,
+		ConfigID: conf.ID,
+		Status:   model.StatusRunning,
+		Commit:   "85f8c029b902ed9400bc600bac301a0aadb144ac",
+	})
+
+	if ok, _ := s.ConfigFindApproved(conf); ok == false {
+		t.Errorf("Want config approved, when running.")
+		return
 	}
 }
 
-//
-// func TestConfigIndexes(t *testing.T) {
-// 	s := newTest()
-// 	defer func() {
-// 		s.Exec("delete from config")
-// 		s.Close()
-// 	}()
-//
-// 	if err := s.FileCreate(
-// 		&model.File{
-// 			BuildID: 1,
-// 			ProcID:  1,
-// 			Name:    "hello.txt",
-// 			Size:    11,
-// 			Mime:    "text/plain",
-// 		},
-// 		bytes.NewBufferString("hello world"),
-// 	); err != nil {
-// 		t.Errorf("Unexpected error: insert file: %s", err)
-// 		return
-// 	}
-//
-// 	// fail due to duplicate file name
-// 	if err := s.FileCreate(
-// 		&model.File{
-// 			BuildID: 1,
-// 			ProcID:  1,
-// 			Name:    "hello.txt",
-// 			Mime:    "text/plain",
-// 			Size:    11,
-// 		},
-// 		bytes.NewBufferString("hello world"),
-// 	); err == nil {
-// 		t.Errorf("Unexpected error: dupliate pid")
-// 	}
-// }
+func TestConfigIndexes(t *testing.T) {
+	s := newTest()
+	defer func() {
+		s.Exec("delete from config")
+		s.Close()
+	}()
+
+	var (
+		data = "pipeline: [ { image: golang, commands: [ go build, go test ] } ]"
+		hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26"
+	)
+
+	if err := s.ConfigCreate(
+		&model.Config{
+			RepoID: 2,
+			Data:   data,
+			Hash:   hash,
+		},
+	); err != nil {
+		t.Errorf("Unexpected error: insert config: %s", err)
+		return
+	}
+
+	// fail due to duplicate sha
+	if err := s.ConfigCreate(
+		&model.Config{
+			RepoID: 2,
+			Data:   data,
+			Hash:   hash,
+		},
+	); err == nil {
+		t.Errorf("Unexpected error: dupliate sha")
+	}
+}

store/datastore/ddl/mysql/16.sql

@@ -5,7 +5,6 @@ CREATE TABLE config (
 ,config_repo_id  INTEGER
 ,config_hash     VARCHAR(250)
 ,config_data     MEDIUMBLOB
-,config_approved BOOLEAN
 
 ,UNIQUE(config_hash, config_repo_id)
 );

store/datastore/ddl/postgres/16.sql

@@ -5,7 +5,6 @@ CREATE TABLE config (
 ,config_repo_id  INTEGER
 ,config_hash     VARCHAR(250)
 ,config_data     BYTEA
-,config_approved BOOLEAN
 
 ,UNIQUE(config_hash, config_repo_id)
 );

store/datastore/ddl/sqlite3/16.sql

@@ -5,7 +5,6 @@ CREATE TABLE config (
 ,config_repo_id  INTEGER
 ,config_hash     TEXT
 ,config_data     BLOB
-,config_approved BOOLEAN
 
 ,UNIQUE(config_hash, config_repo_id)
 );

store/datastore/sql/postgres/files/config.sql

@@ -5,7 +5,6 @@ SELECT
 ,config_repo_id
 ,config_hash
 ,config_data
-,config_approved
 FROM config
 WHERE config_id = $1
 
@@ -16,7 +15,14 @@ SELECT
 ,config_repo_id
 ,config_hash
 ,config_data
-,config_approved
 FROM config
 WHERE config_repo_id = $1
   AND config_hash    = $2
+
+-- name: config-find-approved
+
+SELECT build_id FROM builds
+WHERE build_repo_id = $1
+AND build_config_id = $2
+AND build_status NOT IN ('blocked', 'pending')
+LIMIT 1