Merge pull request #7 from mihaelabalutoiu/refactor-code

Refactor code to allow more unit testing

params/params.go

@@ -150,8 +150,7 @@ type Repository struct {
 	Pools           []Pool `json:"pool,omitempty"`
 	CredentialsName string `json:"credentials_name"`
 	// Do not serialize sensitive info.
-	WebhookSecret string   `json:"-"`
-	Internal      Internal `json:"-"`
+	WebhookSecret string `json:"-"`
 }
 
 type Organization struct {
@@ -160,8 +159,7 @@ type Organization struct {
 	Pools           []Pool `json:"pool,omitempty"`
 	CredentialsName string `json:"credentials_name"`
 	// Do not serialize sensitive info.
-	WebhookSecret string   `json:"-"`
-	Internal      Internal `json:"-"`
+	WebhookSecret string `json:"-"`
 }
 
 // Users holds information about a particular user
@@ -200,5 +198,4 @@ type Provider struct {
 
 type UpdatePoolStateParams struct {
 	WebhookSecret string
-	Internal      Internal
 }

params/requests.go

@@ -57,7 +57,7 @@ type CreateOrgParams struct {
 
 func (c *CreateOrgParams) Validate() error {
 	if c.Name == "" {
-		return errors.NewBadRequestError("missing repo name")
+		return errors.NewBadRequestError("missing org name")
 	}
 
 	if c.CredentialsName == "" {

runner/interfaces.go

@@ -0,0 +1,34 @@
+// Copyright 2022 Cloudbase Solutions SRL
+//
+//    Licensed under the Apache License, Version 2.0 (the "License"); you may
+//    not use this file except in compliance with the License. You may obtain
+//    a copy of the License at
+//
+//         http://www.apache.org/licenses/LICENSE-2.0
+//
+//    Unless required by applicable law or agreed to in writing, software
+//    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+//    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+//    License for the specific language governing permissions and limitations
+//    under the License.
+
+package runner
+
+import (
+	"context"
+	dbCommon "garm/database/common"
+	"garm/params"
+	"garm/runner/common"
+)
+
+type PoolManagerController interface {
+	CreateRepoPoolManager(ctx context.Context, repo params.Repository, providers map[string]common.Provider, store dbCommon.Store) (common.PoolManager, error)
+	GetRepoPoolManager(repo params.Repository) (common.PoolManager, error)
+	DeleteRepoPoolManager(repo params.Repository) error
+	GetRepoPoolManagers() (map[string]common.PoolManager, error)
+
+	CreateOrgPoolManager(ctx context.Context, org params.Organization, providers map[string]common.Provider, store dbCommon.Store) (common.PoolManager, error)
+	GetOrgPoolManager(org params.Organization) (common.PoolManager, error)
+	DeleteOrgPoolManager(org params.Organization) error
+	GetOrgPoolManagers() (map[string]common.PoolManager, error)
+}

runner/organizations.go

@@ -24,7 +24,6 @@ import (
 	runnerErrors "garm/errors"
 	"garm/params"
 	"garm/runner/common"
-	"garm/runner/pool"
 
 	"github.com/pkg/errors"
 )
@@ -46,7 +45,7 @@ func (r *Runner) CreateOrganization(ctx context.Context, param params.CreateOrgP
 	_, err = r.store.GetOrganization(ctx, param.Name)
 	if err != nil {
 		if !errors.Is(err, runnerErrors.ErrNotFound) {
-			return params.Organization{}, errors.Wrap(err, "fetching repo")
+			return params.Organization{}, errors.Wrap(err, "fetching org")
 		}
 	} else {
 		return params.Organization{}, runnerErrors.NewConflictError("organization %s already exists", param.Name)
@@ -63,11 +62,16 @@ func (r *Runner) CreateOrganization(ctx context.Context, param params.CreateOrgP
 		}
 	}()
 
-	poolMgr, err := r.loadOrgPoolManager(org)
+	poolMgr, err := r.poolManagerCtrl.CreateOrgPoolManager(r.ctx, org, r.providers, r.store)
+	if err != nil {
+		return params.Organization{}, errors.Wrap(err, "creating org pool manager")
+	}
 	if err := poolMgr.Start(); err != nil {
-		return params.Organization{}, errors.Wrap(err, "starting pool manager")
+		if deleteErr := r.poolManagerCtrl.DeleteOrgPoolManager(org); deleteErr != nil {
+			log.Printf("failed to cleanup pool manager for org %s", org.ID)
+		}
+		return params.Organization{}, errors.Wrap(err, "starting org pool manager")
 	}
-	r.organizations[org.ID] = poolMgr
 	return org, nil
 }
 
@@ -91,7 +95,7 @@ func (r *Runner) GetOrganizationByID(ctx context.Context, orgID string) (params.
 
 	org, err := r.store.GetOrganizationByID(ctx, orgID)
 	if err != nil {
-		return params.Organization{}, errors.Wrap(err, "fetching repository")
+		return params.Organization{}, errors.Wrap(err, "fetching organization")
 	}
 	return org, nil
 }
@@ -103,20 +107,12 @@ func (r *Runner) DeleteOrganization(ctx context.Context, orgID string) error {
 
 	org, err := r.store.GetOrganizationByID(ctx, orgID)
 	if err != nil {
-		return errors.Wrap(err, "fetching repo")
-	}
-
-	poolMgr, ok := r.organizations[org.ID]
-	if ok {
-		if err := poolMgr.Stop(); err != nil {
-			log.Printf("failed to stop pool for repo %s", org.ID)
-		}
-		delete(r.organizations, orgID)
+		return errors.Wrap(err, "fetching org")
 	}
 
 	pools, err := r.store.ListOrgPools(ctx, orgID)
 	if err != nil {
-		return errors.Wrap(err, "fetching repo pools")
+		return errors.Wrap(err, "fetching org pools")
 	}
 
 	if len(pools) > 0 {
@@ -125,7 +121,11 @@ func (r *Runner) DeleteOrganization(ctx context.Context, orgID string) error {
 			poolIds = append(poolIds, pool.ID)
 		}
 
-		return runnerErrors.NewBadRequestError("repo has pools defined (%s)", strings.Join(poolIds, ", "))
+		return runnerErrors.NewBadRequestError("org has pools defined (%s)", strings.Join(poolIds, ", "))
+	}
+
+	if err := r.poolManagerCtrl.DeleteOrgPoolManager(org); err != nil {
+		return errors.Wrap(err, "deleting org pool manager")
 	}
 
 	if err := r.store.DeleteOrganization(ctx, orgID); err != nil {
@@ -159,27 +159,19 @@ func (r *Runner) UpdateOrganization(ctx context.Context, orgID string, param par
 		return params.Organization{}, errors.Wrap(err, "updating org")
 	}
 
-	poolMgr, ok := r.organizations[org.ID]
-	if ok {
-		internalCfg, err := r.getInternalConfig(org.CredentialsName)
-		if err != nil {
-			return params.Organization{}, errors.Wrap(err, "fetching internal config")
-		}
+	poolMgr, err := r.poolManagerCtrl.GetOrgPoolManager(org)
+	if err != nil {
 		newState := params.UpdatePoolStateParams{
 			WebhookSecret: org.WebhookSecret,
-			Internal:      internalCfg,
 		}
-		org.Internal = internalCfg
 		// stop the pool mgr
 		if err := poolMgr.RefreshState(newState); err != nil {
-			return params.Organization{}, errors.Wrap(err, "updating pool manager")
+			return params.Organization{}, errors.Wrap(err, "updating org pool manager")
 		}
 	} else {
-		poolMgr, err := r.loadOrgPoolManager(org)
-		if err != nil {
-			return params.Organization{}, errors.Wrap(err, "loading pool manager")
+		if _, err := r.poolManagerCtrl.CreateOrgPoolManager(r.ctx, org, r.providers, r.store); err != nil {
+			return params.Organization{}, errors.Wrap(err, "creating org pool manager")
 		}
-		r.organizations[org.ID] = poolMgr
 	}
 
 	return org, nil
@@ -193,8 +185,12 @@ func (r *Runner) CreateOrgPool(ctx context.Context, orgID string, param params.C
 	r.mux.Lock()
 	defer r.mux.Unlock()
 
-	_, ok := r.organizations[orgID]
-	if !ok {
+	org, err := r.store.GetOrganizationByID(ctx, orgID)
+	if err != nil {
+		return params.Pool{}, errors.Wrap(err, "fetching org")
+	}
+
+	if _, err := r.poolManagerCtrl.GetOrgPoolManager(org); err != nil {
 		return params.Pool{}, runnerErrors.ErrNotFound
 	}
 
@@ -313,30 +309,18 @@ func (r *Runner) ListOrgInstances(ctx context.Context, orgID string) ([]params.I
 	return instances, nil
 }
 
-func (r *Runner) loadOrgPoolManager(org params.Organization) (common.PoolManager, error) {
-	cfg, err := r.getInternalConfig(org.CredentialsName)
-	if err != nil {
-		return nil, errors.Wrap(err, "fetching internal config")
-	}
-	org.Internal = cfg
-	poolManager, err := pool.NewOrganizationPoolManager(r.ctx, org, r.providers, r.store)
-	if err != nil {
-		return nil, errors.Wrap(err, "creating pool manager")
-	}
-	return poolManager, nil
-}
-
 func (r *Runner) findOrgPoolManager(name string) (common.PoolManager, error) {
 	r.mux.Lock()
 	defer r.mux.Unlock()
 
 	org, err := r.store.GetOrganization(r.ctx, name)
 	if err != nil {
-		return nil, errors.Wrap(err, "fetching repo")
+		return nil, errors.Wrap(err, "fetching org")
 	}
 
-	if orgPoolMgr, ok := r.organizations[org.ID]; ok {
-		return orgPoolMgr, nil
+	poolManager, err := r.poolManagerCtrl.GetOrgPoolManager(org)
+	if err != nil {
+		return nil, errors.Wrap(err, "fetching pool manager for org")
 	}
-	return nil, errors.Wrapf(runnerErrors.ErrNotFound, "organization %s not configured", name)
+	return poolManager, nil
 }

runner/pool/organization.go

@@ -33,25 +33,26 @@ import (
 // test that we implement PoolManager
 var _ poolHelper = &organization{}
 
-func NewOrganizationPoolManager(ctx context.Context, cfg params.Organization, providers map[string]common.Provider, store dbCommon.Store) (common.PoolManager, error) {
-	ghc, err := util.GithubClient(ctx, cfg.Internal.OAuth2Token)
+func NewOrganizationPoolManager(ctx context.Context, cfg params.Organization, cfgInternal params.Internal, providers map[string]common.Provider, store dbCommon.Store) (common.PoolManager, error) {
+	ghc, err := util.GithubClient(ctx, cfgInternal.OAuth2Token)
 	if err != nil {
 		return nil, errors.Wrap(err, "getting github client")
 	}
 
 	helper := &organization{
-		cfg:   cfg,
-		ctx:   ctx,
-		ghcli: ghc,
-		id:    cfg.ID,
-		store: store,
+		cfg:         cfg,
+		cfgInternal: cfgInternal,
+		ctx:         ctx,
+		ghcli:       ghc,
+		id:          cfg.ID,
+		store:       store,
 	}
 
 	repo := &basePool{
 		ctx:          ctx,
 		store:        store,
 		providers:    providers,
-		controllerID: cfg.Internal.ControllerID,
+		controllerID: cfgInternal.ControllerID,
 		quit:         make(chan struct{}),
 		done:         make(chan struct{}),
 		helper:       helper,
@@ -60,11 +61,12 @@ func NewOrganizationPoolManager(ctx context.Context, cfg params.Organization, pr
 }
 
 type organization struct {
-	cfg   params.Organization
-	ctx   context.Context
-	ghcli common.GithubClient
-	id    string
-	store dbCommon.Store
+	cfg         params.Organization
+	cfgInternal params.Internal
+	ctx         context.Context
+	ghcli       common.GithubClient
+	id          string
+	store       dbCommon.Store
 
 	mux sync.Mutex
 }
@@ -74,7 +76,6 @@ func (r *organization) UpdateState(param params.UpdatePoolStateParams) error {
 	defer r.mux.Unlock()
 
 	r.cfg.WebhookSecret = param.WebhookSecret
-	r.cfg.Internal = param.Internal
 
 	ghc, err := util.GithubClient(r.ctx, r.GetGithubToken())
 	if err != nil {
@@ -85,7 +86,7 @@ func (r *organization) UpdateState(param params.UpdatePoolStateParams) error {
 }
 
 func (r *organization) GetGithubToken() string {
-	return r.cfg.Internal.OAuth2Token
+	return r.cfgInternal.OAuth2Token
 }
 
 func (r *organization) GetGithubRunners() ([]*github.Runner, error) {
@@ -129,7 +130,7 @@ func (r *organization) GithubURL() string {
 }
 
 func (r *organization) JwtToken() string {
-	return r.cfg.Internal.JWTSecret
+	return r.cfgInternal.JWTSecret
 }
 
 func (r *organization) GetGithubRegistrationToken() (string, error) {
@@ -150,7 +151,7 @@ func (r *organization) WebhookSecret() string {
 }
 
 func (r *organization) GetCallbackURL() string {
-	return r.cfg.Internal.InstanceCallbackURL
+	return r.cfgInternal.InstanceCallbackURL
 }
 
 func (r *organization) FindPoolByTags(labels []string) (params.Pool, error) {