From edd2f0bd9fcd38f855def3622bbdc06a7c793236 Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Mon, 18 Mar 2024 22:10:59 +0100 Subject: [PATCH 01/14] Add filter options to GetPipelines --- server/api/pipeline.go | 11 ++++++----- server/model/pipeline.go | 2 +- server/store/datastore/pipeline.go | 18 +++++++----------- server/store/datastore/pipeline_test.go | 2 +- server/store/mocks/store.go | 8 ++++---- server/store/store.go | 2 +- 6 files changed, 20 insertions(+), 23 deletions(-) diff --git a/server/api/pipeline.go b/server/api/pipeline.go index 1bb8725d74..a89ee53ccc 100644 --- a/server/api/pipeline.go +++ b/server/api/pipeline.go @@ -109,19 +109,20 @@ func createTmpPipeline(event model.WebhookEvent, commit *model.Commit, user *mod // @Param repo_id path int true "the repository id" // @Param page query int false "for response pagination, page offset number" default(1) // @Param perPage query int false "for response pagination, max items per page" default(50) -// @Param before query string false "only return pipelines before this RFC3339 date" -// @Param after query string false "only return pipelines after this RFC3339 date" +// @Param before query string false "only return pipelines before this RFC3339 date" default(time.Now) func GetPipelines(c *gin.Context) { repo := session.Repo(c) before := c.Query("before") after := c.Query("after") - filter := new(model.PipelineFilter) + filter := &model.FilterOptions{ + Before: time.Now().UTC().Unix(), + } if before != "" { beforeDt, err := time.Parse(time.RFC3339, before) if err != nil { - _ = c.AbortWithError(http.StatusBadRequest, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } filter.Before = beforeDt.Unix() @@ -130,7 +131,7 @@ func GetPipelines(c *gin.Context) { if after != "" { afterDt, err := time.Parse(time.RFC3339, after) if err != nil { - _ = c.AbortWithError(http.StatusBadRequest, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } filter.After = afterDt.Unix() diff --git a/server/model/pipeline.go b/server/model/pipeline.go index 9454b40e54..f841f80488 100644 --- a/server/model/pipeline.go +++ b/server/model/pipeline.go @@ -53,7 +53,7 @@ type Pipeline struct { IsPrerelease bool `json:"is_prerelease,omitempty" xorm:"is_prerelease"` } // @name Pipeline -type PipelineFilter struct { +type FilterOptions struct { Before int64 After int64 } diff --git a/server/store/datastore/pipeline.go b/server/store/datastore/pipeline.go index 4ed6e9022e..f315c7f846 100644 --- a/server/store/datastore/pipeline.go +++ b/server/store/datastore/pipeline.go @@ -52,22 +52,18 @@ func (s storage) GetPipelineLastBefore(repo *model.Repo, branch string, num int6 Get(pipeline)) } -func (s storage) GetPipelineList(repo *model.Repo, p *model.ListOptions, f *model.PipelineFilter) ([]*model.Pipeline, error) { +func (s storage) GetPipelineList(repo *model.Repo, p *model.ListOptions, f *model.FilterOptions) ([]*model.Pipeline, error) { pipelines := make([]*model.Pipeline, 0, 16) - cond := builder.NewCond().And(builder.Eq{"pipeline_repo_id": repo.ID}) - if f != nil { - if f.After != 0 { - cond = cond.And(builder.Gt{"pipeline_started": f.After}) - } - - if f.Before != 0 { - cond = cond.And(builder.Lt{"pipeline_started": f.Before}) - } + return pipelines, s.paginate(p).Where(builder.Eq{"pipeline_repo_id": repo.ID}). + And(builder.Gt{"pipeline_started": f.After}). + And(builder.Lt{"pipeline_started": f.Before}). + Desc("pipeline_number"). + Find(&pipelines) } - return pipelines, s.paginate(p).Where(cond). + return pipelines, s.paginate(p).Where(builder.Eq{"pipeline_repo_id": repo.ID}). Desc("pipeline_number"). Find(&pipelines) } diff --git a/server/store/datastore/pipeline_test.go b/server/store/datastore/pipeline_test.go index df1ead63f9..861e432cea 100644 --- a/server/store/datastore/pipeline_test.go +++ b/server/store/datastore/pipeline_test.go @@ -245,7 +245,7 @@ func TestPipelines(t *testing.T) { g.Assert(err1).IsNil() err2 := store.CreatePipeline(pipeline2, []*model.Step{}...) g.Assert(err2).IsNil() - pipelines, err3 := store.GetPipelineList(&model.Repo{ID: 1}, &model.ListOptions{Page: 1, PerPage: 50}, &model.PipelineFilter{Before: dt2.Unix()}) + pipelines, err3 := store.GetPipelineList(&model.Repo{ID: 1}, &model.ListOptions{Page: 1, PerPage: 50}, &model.FilterOptions{Before: dt2.Unix()}) g.Assert(err3).IsNil() g.Assert(len(pipelines)).Equal(1) g.Assert(pipelines[0].ID).Equal(pipeline1.ID) diff --git a/server/store/mocks/store.go b/server/store/mocks/store.go index ee1a825d08..e37b156a68 100644 --- a/server/store/mocks/store.go +++ b/server/store/mocks/store.go @@ -802,7 +802,7 @@ func (_m *Store) GetPipelineLastBefore(_a0 *model.Repo, _a1 string, _a2 int64) ( } // GetPipelineList provides a mock function with given fields: _a0, _a1, _a2 -func (_m *Store) GetPipelineList(_a0 *model.Repo, _a1 *model.ListOptions, _a2 *model.PipelineFilter) ([]*model.Pipeline, error) { +func (_m *Store) GetPipelineList(_a0 *model.Repo, _a1 *model.ListOptions, _a2 *model.FilterOptions) ([]*model.Pipeline, error) { ret := _m.Called(_a0, _a1, _a2) if len(ret) == 0 { @@ -811,10 +811,10 @@ func (_m *Store) GetPipelineList(_a0 *model.Repo, _a1 *model.ListOptions, _a2 *m var r0 []*model.Pipeline var r1 error - if rf, ok := ret.Get(0).(func(*model.Repo, *model.ListOptions, *model.PipelineFilter) ([]*model.Pipeline, error)); ok { + if rf, ok := ret.Get(0).(func(*model.Repo, *model.ListOptions, *model.FilterOptions) ([]*model.Pipeline, error)); ok { return rf(_a0, _a1, _a2) } - if rf, ok := ret.Get(0).(func(*model.Repo, *model.ListOptions, *model.PipelineFilter) []*model.Pipeline); ok { + if rf, ok := ret.Get(0).(func(*model.Repo, *model.ListOptions, *model.FilterOptions) []*model.Pipeline); ok { r0 = rf(_a0, _a1, _a2) } else { if ret.Get(0) != nil { @@ -822,7 +822,7 @@ func (_m *Store) GetPipelineList(_a0 *model.Repo, _a1 *model.ListOptions, _a2 *m } } - if rf, ok := ret.Get(1).(func(*model.Repo, *model.ListOptions, *model.PipelineFilter) error); ok { + if rf, ok := ret.Get(1).(func(*model.Repo, *model.ListOptions, *model.FilterOptions) error); ok { r1 = rf(_a0, _a1, _a2) } else { r1 = ret.Error(1) diff --git a/server/store/store.go b/server/store/store.go index 2a007e487c..b4be0cdfa5 100644 --- a/server/store/store.go +++ b/server/store/store.go @@ -75,7 +75,7 @@ type Store interface { // GetPipelineLastBefore gets the last pipeline before pipeline number N. GetPipelineLastBefore(*model.Repo, string, int64) (*model.Pipeline, error) // GetPipelineList gets a list of pipelines for the repository - GetPipelineList(*model.Repo, *model.ListOptions, *model.PipelineFilter) ([]*model.Pipeline, error) + GetPipelineList(*model.Repo, *model.ListOptions, *model.FilterOptions) ([]*model.Pipeline, error) // GetActivePipelineList gets a list of the active pipelines for the repository GetActivePipelineList(repo *model.Repo) ([]*model.Pipeline, error) // GetPipelineQueue gets a list of pipelines in queue. From 0ae9adf3b400c2541b472aa0f2a2a98fa580f589 Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Mon, 18 Mar 2024 22:14:48 +0100 Subject: [PATCH 02/14] fix query docs --- server/api/pipeline.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/api/pipeline.go b/server/api/pipeline.go index a89ee53ccc..2d148c8fe3 100644 --- a/server/api/pipeline.go +++ b/server/api/pipeline.go @@ -109,7 +109,8 @@ func createTmpPipeline(event model.WebhookEvent, commit *model.Commit, user *mod // @Param repo_id path int true "the repository id" // @Param page query int false "for response pagination, page offset number" default(1) // @Param perPage query int false "for response pagination, max items per page" default(50) -// @Param before query string false "only return pipelines before this RFC3339 date" default(time.Now) +// @Param before query string false "only return pipelines before this RFC3339 date" +// @Param after query string false "only return pipelines after this RFC3339 date" func GetPipelines(c *gin.Context) { repo := session.Repo(c) before := c.Query("before") From 42e12e1ff914fa1bea8f19a2b1c79756bc40d19c Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Tue, 19 Mar 2024 16:34:12 +0100 Subject: [PATCH 03/14] use builder condition --- server/store/datastore/pipeline.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/server/store/datastore/pipeline.go b/server/store/datastore/pipeline.go index f315c7f846..fc8dcf0227 100644 --- a/server/store/datastore/pipeline.go +++ b/server/store/datastore/pipeline.go @@ -55,15 +55,18 @@ func (s storage) GetPipelineLastBefore(repo *model.Repo, branch string, num int6 func (s storage) GetPipelineList(repo *model.Repo, p *model.ListOptions, f *model.FilterOptions) ([]*model.Pipeline, error) { pipelines := make([]*model.Pipeline, 0, 16) + cond := builder.NewCond() + cond = cond.And(builder.Eq{"pipeline_repo_id": repo.ID}) + if f != nil { - return pipelines, s.paginate(p).Where(builder.Eq{"pipeline_repo_id": repo.ID}). - And(builder.Gt{"pipeline_started": f.After}). - And(builder.Lt{"pipeline_started": f.Before}). - Desc("pipeline_number"). - Find(&pipelines) + cond = cond.And(builder.Gt{"pipeline_started": f.After}) + + if f.Before != 0 { + cond = cond.And(builder.Lt{"pipeline_started": f.Before}) + } } - return pipelines, s.paginate(p).Where(builder.Eq{"pipeline_repo_id": repo.ID}). + return pipelines, s.paginate(p).Where(cond). Desc("pipeline_number"). Find(&pipelines) } From f0628bd1d792d81c1e191988bb7ad8c4183ad4ff Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Tue, 19 Mar 2024 16:58:33 +0100 Subject: [PATCH 04/14] Update server/store/datastore/pipeline.go Co-authored-by: 6543 --- server/store/datastore/pipeline.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/store/datastore/pipeline.go b/server/store/datastore/pipeline.go index fc8dcf0227..d38c246ea9 100644 --- a/server/store/datastore/pipeline.go +++ b/server/store/datastore/pipeline.go @@ -55,8 +55,7 @@ func (s storage) GetPipelineLastBefore(repo *model.Repo, branch string, num int6 func (s storage) GetPipelineList(repo *model.Repo, p *model.ListOptions, f *model.FilterOptions) ([]*model.Pipeline, error) { pipelines := make([]*model.Pipeline, 0, 16) - cond := builder.NewCond() - cond = cond.And(builder.Eq{"pipeline_repo_id": repo.ID}) + cond := builder.NewCond().And(builder.Eq{"pipeline_repo_id": repo.ID}) if f != nil { cond = cond.And(builder.Gt{"pipeline_started": f.After}) From dbd202563ed14f02db1b2445e6bca1279cbad359 Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Tue, 19 Mar 2024 16:58:45 +0100 Subject: [PATCH 05/14] Update server/api/pipeline.go Co-authored-by: 6543 --- server/api/pipeline.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/server/api/pipeline.go b/server/api/pipeline.go index 2d148c8fe3..20003665d2 100644 --- a/server/api/pipeline.go +++ b/server/api/pipeline.go @@ -116,9 +116,7 @@ func GetPipelines(c *gin.Context) { before := c.Query("before") after := c.Query("after") - filter := &model.FilterOptions{ - Before: time.Now().UTC().Unix(), - } + filter := new(model.FilterOptions) if before != "" { beforeDt, err := time.Parse(time.RFC3339, before) From 731354567322f994fbc281d5bcaeb67751e88dbc Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Tue, 19 Mar 2024 17:07:46 +0100 Subject: [PATCH 06/14] rename FilterOptions and add DeletePipelines api --- server/api/pipeline.go | 64 ++++++++++++++++++++++++- server/model/pipeline.go | 2 +- server/router/api.go | 1 + server/store/datastore/pipeline.go | 10 ++-- server/store/datastore/pipeline_test.go | 2 +- server/store/mocks/store.go | 8 ++-- server/store/store.go | 2 +- 7 files changed, 75 insertions(+), 14 deletions(-) diff --git a/server/api/pipeline.go b/server/api/pipeline.go index 20003665d2..981a34291d 100644 --- a/server/api/pipeline.go +++ b/server/api/pipeline.go @@ -116,7 +116,7 @@ func GetPipelines(c *gin.Context) { before := c.Query("before") after := c.Query("after") - filter := new(model.FilterOptions) + filter := new(model.PipelineFilter) if before != "" { beforeDt, err := time.Parse(time.RFC3339, before) @@ -144,6 +144,66 @@ func GetPipelines(c *gin.Context) { c.JSON(http.StatusOK, pipelines) } +// DeletePipelines +// +// @Summary Delete pipelines +// @Router /repos/{repo_id}/pipelines [delete] +// @Produce plain +// @Success 204 +// @Tags Pipelines +// @Param Authorization header string true "Insert your personal access token" default(Bearer ) +// @Param repo_id path int true "the repository id" +// @Param page query int false "for response pagination, page offset number" default(1) +// @Param perPage query int false "for response pagination, max items per page" default(50) +// @Param before query string false "only return pipelines before this RFC3339 date" +// @Param after query string false "only return pipelines after this RFC3339 date" +func DeletePipelines(c *gin.Context) { + repo := session.Repo(c) + before := c.Query("before") + after := c.Query("after") + + filter := &model.PipelineFilter{ + Before: time.Now().UTC().Unix(), + } + + if before != "" { + beforeDt, err := time.Parse(time.RFC3339, before) + if err != nil { + _ = c.AbortWithError(http.StatusInternalServerError, err) + return + } + filter.Before = beforeDt.Unix() + } + + if after != "" { + afterDt, err := time.Parse(time.RFC3339, after) + if err != nil { + _ = c.AbortWithError(http.StatusInternalServerError, err) + return + } + filter.After = afterDt.Unix() + } + + pipelines, err := store.FromContext(c).GetPipelineList(repo, session.Pagination(c), filter) + if err != nil { + _ = c.AbortWithError(http.StatusInternalServerError, err) + return + } + + for _, p := range pipelines { + if delWErr := store.FromContext(c).DeletePipeline(p); err != nil { + err = errors.Join(err, delWErr) + } + } + + if err != nil { + c.String(http.StatusInternalServerError, "Error deleting pipelines. %s", err) + return + } + + c.Status(http.StatusNoContent) +} + // GetPipeline // // @Summary Pipeline information by number @@ -586,7 +646,7 @@ func DeletePipelineLogs(c *gin.Context) { } } if err != nil { - c.String(http.StatusInternalServerError, "There was a problem deleting your logs. %s", err) + c.String(http.StatusInternalServerError, "Error deleting pipeline logs. %s", err) return } diff --git a/server/model/pipeline.go b/server/model/pipeline.go index f841f80488..9454b40e54 100644 --- a/server/model/pipeline.go +++ b/server/model/pipeline.go @@ -53,7 +53,7 @@ type Pipeline struct { IsPrerelease bool `json:"is_prerelease,omitempty" xorm:"is_prerelease"` } // @name Pipeline -type FilterOptions struct { +type PipelineFilter struct { Before int64 After int64 } diff --git a/server/router/api.go b/server/router/api.go index 29926987a9..9ac461bfa3 100644 --- a/server/router/api.go +++ b/server/router/api.go @@ -94,6 +94,7 @@ func apiRoutes(e *gin.RouterGroup) { repo.GET("/pipelines", api.GetPipelines) repo.POST("/pipelines", session.MustPush, api.CreatePipeline) + repo.DELETE("/pipelines", api.DeletePipelines) repo.GET("/pipelines/:number", api.GetPipeline) repo.GET("/pipelines/:number/config", api.GetPipelineConfig) diff --git a/server/store/datastore/pipeline.go b/server/store/datastore/pipeline.go index d38c246ea9..ab85804656 100644 --- a/server/store/datastore/pipeline.go +++ b/server/store/datastore/pipeline.go @@ -52,17 +52,17 @@ func (s storage) GetPipelineLastBefore(repo *model.Repo, branch string, num int6 Get(pipeline)) } -func (s storage) GetPipelineList(repo *model.Repo, p *model.ListOptions, f *model.FilterOptions) ([]*model.Pipeline, error) { +func (s storage) GetPipelineList(repo *model.Repo, p *model.ListOptions, f *model.PipelineFilter) ([]*model.Pipeline, error) { pipelines := make([]*model.Pipeline, 0, 16) cond := builder.NewCond().And(builder.Eq{"pipeline_repo_id": repo.ID}) - if f != nil { + if f.After != 0 { cond = cond.And(builder.Gt{"pipeline_started": f.After}) + } - if f.Before != 0 { - cond = cond.And(builder.Lt{"pipeline_started": f.Before}) - } + if f.Before != 0 { + cond = cond.And(builder.Lt{"pipeline_started": f.Before}) } return pipelines, s.paginate(p).Where(cond). diff --git a/server/store/datastore/pipeline_test.go b/server/store/datastore/pipeline_test.go index 861e432cea..df1ead63f9 100644 --- a/server/store/datastore/pipeline_test.go +++ b/server/store/datastore/pipeline_test.go @@ -245,7 +245,7 @@ func TestPipelines(t *testing.T) { g.Assert(err1).IsNil() err2 := store.CreatePipeline(pipeline2, []*model.Step{}...) g.Assert(err2).IsNil() - pipelines, err3 := store.GetPipelineList(&model.Repo{ID: 1}, &model.ListOptions{Page: 1, PerPage: 50}, &model.FilterOptions{Before: dt2.Unix()}) + pipelines, err3 := store.GetPipelineList(&model.Repo{ID: 1}, &model.ListOptions{Page: 1, PerPage: 50}, &model.PipelineFilter{Before: dt2.Unix()}) g.Assert(err3).IsNil() g.Assert(len(pipelines)).Equal(1) g.Assert(pipelines[0].ID).Equal(pipeline1.ID) diff --git a/server/store/mocks/store.go b/server/store/mocks/store.go index e37b156a68..ee1a825d08 100644 --- a/server/store/mocks/store.go +++ b/server/store/mocks/store.go @@ -802,7 +802,7 @@ func (_m *Store) GetPipelineLastBefore(_a0 *model.Repo, _a1 string, _a2 int64) ( } // GetPipelineList provides a mock function with given fields: _a0, _a1, _a2 -func (_m *Store) GetPipelineList(_a0 *model.Repo, _a1 *model.ListOptions, _a2 *model.FilterOptions) ([]*model.Pipeline, error) { +func (_m *Store) GetPipelineList(_a0 *model.Repo, _a1 *model.ListOptions, _a2 *model.PipelineFilter) ([]*model.Pipeline, error) { ret := _m.Called(_a0, _a1, _a2) if len(ret) == 0 { @@ -811,10 +811,10 @@ func (_m *Store) GetPipelineList(_a0 *model.Repo, _a1 *model.ListOptions, _a2 *m var r0 []*model.Pipeline var r1 error - if rf, ok := ret.Get(0).(func(*model.Repo, *model.ListOptions, *model.FilterOptions) ([]*model.Pipeline, error)); ok { + if rf, ok := ret.Get(0).(func(*model.Repo, *model.ListOptions, *model.PipelineFilter) ([]*model.Pipeline, error)); ok { return rf(_a0, _a1, _a2) } - if rf, ok := ret.Get(0).(func(*model.Repo, *model.ListOptions, *model.FilterOptions) []*model.Pipeline); ok { + if rf, ok := ret.Get(0).(func(*model.Repo, *model.ListOptions, *model.PipelineFilter) []*model.Pipeline); ok { r0 = rf(_a0, _a1, _a2) } else { if ret.Get(0) != nil { @@ -822,7 +822,7 @@ func (_m *Store) GetPipelineList(_a0 *model.Repo, _a1 *model.ListOptions, _a2 *m } } - if rf, ok := ret.Get(1).(func(*model.Repo, *model.ListOptions, *model.FilterOptions) error); ok { + if rf, ok := ret.Get(1).(func(*model.Repo, *model.ListOptions, *model.PipelineFilter) error); ok { r1 = rf(_a0, _a1, _a2) } else { r1 = ret.Error(1) diff --git a/server/store/store.go b/server/store/store.go index b4be0cdfa5..2a007e487c 100644 --- a/server/store/store.go +++ b/server/store/store.go @@ -75,7 +75,7 @@ type Store interface { // GetPipelineLastBefore gets the last pipeline before pipeline number N. GetPipelineLastBefore(*model.Repo, string, int64) (*model.Pipeline, error) // GetPipelineList gets a list of pipelines for the repository - GetPipelineList(*model.Repo, *model.ListOptions, *model.FilterOptions) ([]*model.Pipeline, error) + GetPipelineList(*model.Repo, *model.ListOptions, *model.PipelineFilter) ([]*model.Pipeline, error) // GetActivePipelineList gets a list of the active pipelines for the repository GetActivePipelineList(repo *model.Repo) ([]*model.Pipeline, error) // GetPipelineQueue gets a list of pipelines in queue. From 3571d817af04a54e06076c114ea03b3b54f8e0b4 Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Tue, 19 Mar 2024 17:11:48 +0100 Subject: [PATCH 07/14] cleanup --- server/api/pipeline.go | 4 +--- server/store/datastore/pipeline.go | 12 +++++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/server/api/pipeline.go b/server/api/pipeline.go index 981a34291d..77e94610ae 100644 --- a/server/api/pipeline.go +++ b/server/api/pipeline.go @@ -162,9 +162,7 @@ func DeletePipelines(c *gin.Context) { before := c.Query("before") after := c.Query("after") - filter := &model.PipelineFilter{ - Before: time.Now().UTC().Unix(), - } + filter := new(model.PipelineFilter) if before != "" { beforeDt, err := time.Parse(time.RFC3339, before) diff --git a/server/store/datastore/pipeline.go b/server/store/datastore/pipeline.go index ab85804656..4ed6e9022e 100644 --- a/server/store/datastore/pipeline.go +++ b/server/store/datastore/pipeline.go @@ -57,12 +57,14 @@ func (s storage) GetPipelineList(repo *model.Repo, p *model.ListOptions, f *mode cond := builder.NewCond().And(builder.Eq{"pipeline_repo_id": repo.ID}) - if f.After != 0 { - cond = cond.And(builder.Gt{"pipeline_started": f.After}) - } + if f != nil { + if f.After != 0 { + cond = cond.And(builder.Gt{"pipeline_started": f.After}) + } - if f.Before != 0 { - cond = cond.And(builder.Lt{"pipeline_started": f.Before}) + if f.Before != 0 { + cond = cond.And(builder.Lt{"pipeline_started": f.Before}) + } } return pipelines, s.paginate(p).Where(cond). From 37f26905ae738e43aeabc053b704253c364d1974 Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Tue, 19 Mar 2024 22:24:36 +0100 Subject: [PATCH 08/14] fix swagger and add api tests --- cmd/server/docs/docs.go | 135 +++++++++++++++++++++++++++++++----- server/api/pipeline_test.go | 37 ++-------- 2 files changed, 123 insertions(+), 49 deletions(-) diff --git a/cmd/server/docs/docs.go b/cmd/server/docs/docs.go index dbe9d53c6e..effbe3b0a6 100644 --- a/cmd/server/docs/docs.go +++ b/cmd/server/docs/docs.go @@ -1,4 +1,5 @@ -// Package docs Code generated by swaggo/swag. DO NOT EDIT +// Code generated by swaggo/swag. DO NOT EDIT. + package docs import "github.com/swaggo/swag" @@ -2242,6 +2243,63 @@ const docTemplate = `{ } } } + }, + "delete": { + "produces": [ + "text/plain" + ], + "tags": [ + "Pipelines" + ], + "summary": "Delete pipelines", + "parameters": [ + { + "type": "string", + "default": "Bearer \u003cpersonal access token\u003e", + "description": "Insert your personal access token", + "name": "Authorization", + "in": "header", + "required": true + }, + { + "type": "integer", + "description": "the repository id", + "name": "repo_id", + "in": "path", + "required": true + }, + { + "type": "integer", + "default": 1, + "description": "for response pagination, page offset number", + "name": "page", + "in": "query" + }, + { + "type": "integer", + "default": 50, + "description": "for response pagination, max items per page", + "name": "perPage", + "in": "query" + }, + { + "type": "string", + "description": "only return pipelines before this RFC3339 date", + "name": "before", + "in": "query" + }, + { + "type": "string", + "description": "only return pipelines after this RFC3339 date", + "name": "after", + "in": "query" + } + ], + "responses": { + "204": { + "description": "No Content" + } + } } }, "/repos/{repo_id}/pipelines/{number}": { @@ -3879,27 +3937,10 @@ const docTemplate = `{ "type": "integer" }, "type": { - "$ref": "#/definitions/LogEntryType" + "$ref": "#/definitions/model.LogEntryType" } } }, - "LogEntryType": { - "type": "integer", - "enum": [ - 0, - 1, - 2, - 3, - 4 - ], - "x-enum-varnames": [ - "LogEntryStdout", - "LogEntryStderr", - "LogEntryExitCode", - "LogEntryMetadata", - "LogEntryProgress" - ] - }, "Org": { "type": "object", "properties": { @@ -4476,6 +4517,62 @@ const docTemplate = `{ "EventManual" ] }, + "errors.PipelineError": { + "type": "object", + "properties": { + "data": {}, + "is_warning": { + "type": "boolean" + }, + "message": { + "type": "string" + }, + "type": { + "$ref": "#/definitions/errors.PipelineErrorType" + } + } + }, + "errors.PipelineErrorType": { + "type": "string", + "enum": [ + "linter", + "deprecation", + "compiler", + "generic", + "bad_habit" + ], + "x-enum-comments": { + "PipelineErrorTypeBadHabit": "some bad-habit error", + "PipelineErrorTypeCompiler": "some error with the config semantics", + "PipelineErrorTypeDeprecation": "using some deprecated feature", + "PipelineErrorTypeGeneric": "some generic error", + "PipelineErrorTypeLinter": "some error with the config syntax" + }, + "x-enum-varnames": [ + "PipelineErrorTypeLinter", + "PipelineErrorTypeDeprecation", + "PipelineErrorTypeCompiler", + "PipelineErrorTypeGeneric", + "PipelineErrorTypeBadHabit" + ] + }, + "model.LogEntryType": { + "type": "integer", + "enum": [ + 0, + 1, + 2, + 3, + 4 + ], + "x-enum-varnames": [ + "LogEntryStdout", + "LogEntryStderr", + "LogEntryExitCode", + "LogEntryMetadata", + "LogEntryProgress" + ] + }, "model.Workflow": { "type": "object", "properties": { diff --git a/server/api/pipeline_test.go b/server/api/pipeline_test.go index 9f0608dc90..0108f33c35 100644 --- a/server/api/pipeline_test.go +++ b/server/api/pipeline_test.go @@ -23,51 +23,28 @@ func TestGetPipelines(t *testing.T) { g := goblin.Goblin(t) g.Describe("Pipeline", func() { - g.It("should get pipelines", func() { - pipelines := []*model.Pipeline{fakePipeline} - - mockStore := mocks.NewStore(t) - mockStore.On("GetPipelineList", mock.Anything, mock.Anything, mock.Anything).Return(pipelines, nil) - - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - c.Set("store", mockStore) - - GetPipelines(c) - - mockStore.AssertCalled(t, "GetPipelineList", mock.Anything, mock.Anything, mock.Anything) - assert.Equal(t, http.StatusOK, c.Writer.Status()) - }) - - g.It("should not parse pipeline filter", func() { - c, _ := gin.CreateTestContext(httptest.NewRecorder()) - c.Request, _ = http.NewRequest("DELETE", "/?before=2023-01-16&after=2023-01-15", nil) - - GetPipelines(c) - - assert.Equal(t, http.StatusBadRequest, c.Writer.Status()) - }) - g.It("should parse pipeline filter", func() { - pipelines := []*model.Pipeline{fakePipeline} + pipelines := make([]*model.Pipeline, 1) mockStore := mocks.NewStore(t) mockStore.On("GetPipelineList", mock.Anything, mock.Anything, mock.Anything).Return(pipelines, nil) + mockStore.On("DeletePipeline", mock.Anything).Return(nil) c, _ := gin.CreateTestContext(httptest.NewRecorder()) c.Set("store", mockStore) - c.Request, _ = http.NewRequest("DELETE", "/?2023-01-16T15:00:00Z&after=2023-01-15T15:00:00Z", nil) + c.Request, _ = http.NewRequest("DELETE", "/?before=2023-01-16T15:00:00Z&after=2023-01-15T15:00:00Z", nil) - GetPipelines(c) + DeletePipelines(c) - assert.Equal(t, http.StatusOK, c.Writer.Status()) + assert.Equal(t, http.StatusNoContent, c.Writer.Status()) }) g.It("should parse pipeline filter with tz offset", func() { - pipelines := []*model.Pipeline{fakePipeline} + pipelines := make([]*model.Pipeline, 1) mockStore := mocks.NewStore(t) mockStore.On("GetPipelineList", mock.Anything, mock.Anything, mock.Anything).Return(pipelines, nil) + mockStore.On("DeletePipeline", mock.Anything).Return(nil) c, _ := gin.CreateTestContext(httptest.NewRecorder()) c.Set("store", mockStore) From 32879f5d438e5966c64d2f38cdd0588c10bb30d3 Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Wed, 20 Mar 2024 12:51:13 +0100 Subject: [PATCH 09/14] add permission check --- server/router/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/router/api.go b/server/router/api.go index 9ac461bfa3..dd3505d826 100644 --- a/server/router/api.go +++ b/server/router/api.go @@ -94,7 +94,7 @@ func apiRoutes(e *gin.RouterGroup) { repo.GET("/pipelines", api.GetPipelines) repo.POST("/pipelines", session.MustPush, api.CreatePipeline) - repo.DELETE("/pipelines", api.DeletePipelines) + repo.DELETE("/pipelines", session.MustRepoAdmin(), api.DeletePipelines) repo.GET("/pipelines/:number", api.GetPipeline) repo.GET("/pipelines/:number/config", api.GetPipelineConfig) From 0891eb33c7341735863e31e365c328843e78a016 Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Wed, 20 Mar 2024 12:54:29 +0100 Subject: [PATCH 10/14] fix swagger --- cmd/server/docs/docs.go | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/cmd/server/docs/docs.go b/cmd/server/docs/docs.go index effbe3b0a6..3e68e31a54 100644 --- a/cmd/server/docs/docs.go +++ b/cmd/server/docs/docs.go @@ -1,5 +1,4 @@ -// Code generated by swaggo/swag. DO NOT EDIT. - +// Package docs Code generated by swaggo/swag. DO NOT EDIT package docs import "github.com/swaggo/swag" @@ -3937,10 +3936,27 @@ const docTemplate = `{ "type": "integer" }, "type": { - "$ref": "#/definitions/model.LogEntryType" + "$ref": "#/definitions/LogEntryType" } } }, + "LogEntryType": { + "type": "integer", + "enum": [ + 0, + 1, + 2, + 3, + 4 + ], + "x-enum-varnames": [ + "LogEntryStdout", + "LogEntryStderr", + "LogEntryExitCode", + "LogEntryMetadata", + "LogEntryProgress" + ] + }, "Org": { "type": "object", "properties": { @@ -4556,23 +4572,6 @@ const docTemplate = `{ "PipelineErrorTypeBadHabit" ] }, - "model.LogEntryType": { - "type": "integer", - "enum": [ - 0, - 1, - 2, - 3, - 4 - ], - "x-enum-varnames": [ - "LogEntryStdout", - "LogEntryStderr", - "LogEntryExitCode", - "LogEntryMetadata", - "LogEntryProgress" - ] - }, "model.Workflow": { "type": "object", "properties": { From c93c73e0696860ca7a5d7b721bc961b423918033 Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Wed, 20 Mar 2024 13:25:41 +0100 Subject: [PATCH 11/14] use status check helper and prevent deletion in some cases --- server/api/helper.go | 11 +++++++++++ server/api/pipeline.go | 14 +++++++++----- server/api/pipeline_test.go | 30 ++++++++++++++++++++++++++++-- 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/server/api/helper.go b/server/api/helper.go index 8b621487b2..12b52efa86 100644 --- a/server/api/helper.go +++ b/server/api/helper.go @@ -64,3 +64,14 @@ func refreshUserToken(c *gin.Context, user *model.User) { } forge.Refresh(c, _forge, _store, user) } + +// pipelineDeleteAllowed checks if the given pipeline can be deleted based on its status. +// It returns a bool indicating if delete is allowed, and the pipeline's status. +func pipelineDeleteAllowed(pl *model.Pipeline) (bool, model.StatusValue) { + switch pl.Status { + case model.StatusRunning, model.StatusPending, model.StatusBlocked: + return false, pl.Status + } + + return true, pl.Status +} diff --git a/server/api/pipeline.go b/server/api/pipeline.go index 77e94610ae..bee44b4e3c 100644 --- a/server/api/pipeline.go +++ b/server/api/pipeline.go @@ -188,8 +188,13 @@ func DeletePipelines(c *gin.Context) { return } - for _, p := range pipelines { - if delWErr := store.FromContext(c).DeletePipeline(p); err != nil { + for _, pl := range pipelines { + if ok, status := pipelineDeleteAllowed(pl); !ok { + c.String(http.StatusUnprocessableEntity, "Cannot delete pipeline with status %s", status) + return + } + + if delWErr := store.FromContext(c).DeletePipeline(pl); err != nil { err = errors.Join(err, delWErr) } } @@ -632,9 +637,8 @@ func DeletePipelineLogs(c *gin.Context) { return } - switch pl.Status { - case model.StatusRunning, model.StatusPending: - c.String(http.StatusUnprocessableEntity, "Cannot delete logs for a pending or running pipeline") + if ok, status := pipelineDeleteAllowed(pl); !ok { + c.String(http.StatusUnprocessableEntity, "Cannot delete logs for pipeline with status %s", status) return } diff --git a/server/api/pipeline_test.go b/server/api/pipeline_test.go index 0108f33c35..17473d009a 100644 --- a/server/api/pipeline_test.go +++ b/server/api/pipeline_test.go @@ -21,10 +21,18 @@ var fakePipeline = &model.Pipeline{ func TestGetPipelines(t *testing.T) { gin.SetMode(gin.TestMode) + pipeline1 := &model.Pipeline{ + Status: model.StatusSuccess, + } + pipeline2 := &model.Pipeline{ + Status: model.StatusPending, + } + g := goblin.Goblin(t) g.Describe("Pipeline", func() { g.It("should parse pipeline filter", func() { - pipelines := make([]*model.Pipeline, 1) + pipelines := make([]*model.Pipeline, 0) + pipelines = append(pipelines, pipeline1) mockStore := mocks.NewStore(t) mockStore.On("GetPipelineList", mock.Anything, mock.Anything, mock.Anything).Return(pipelines, nil) @@ -40,7 +48,8 @@ func TestGetPipelines(t *testing.T) { }) g.It("should parse pipeline filter with tz offset", func() { - pipelines := make([]*model.Pipeline, 1) + pipelines := make([]*model.Pipeline, 0) + pipelines = append(pipelines, pipeline1) mockStore := mocks.NewStore(t) mockStore.On("GetPipelineList", mock.Anything, mock.Anything, mock.Anything).Return(pipelines, nil) @@ -54,5 +63,22 @@ func TestGetPipelines(t *testing.T) { assert.Equal(t, http.StatusOK, c.Writer.Status()) }) + + g.It("should not delete pending", func() { + pipelines := make([]*model.Pipeline, 0) + pipelines = append(pipelines, pipeline2) + + mockStore := mocks.NewStore(t) + mockStore.On("GetPipelineList", mock.Anything, mock.Anything, mock.Anything).Return(pipelines, nil) + + c, _ := gin.CreateTestContext(httptest.NewRecorder()) + c.Set("store", mockStore) + + DeletePipelines(c) + + mockStore.AssertCalled(t, "GetPipelineList", mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "DeletePipeline", mock.Anything) + assert.Equal(t, http.StatusUnprocessableEntity, c.Writer.Status()) + }) }) } From bb1b2fa0927b66754f5b1aeb21c7a5a8a1c2eca2 Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Mon, 22 Apr 2024 14:58:28 +0200 Subject: [PATCH 12/14] refactor to just delete one pipeline --- cmd/server/docs/docs.go | 132 ++++++++++-------------------------- server/api/pipeline.go | 61 ++++++----------- server/api/pipeline_test.go | 87 ++++++++++++++++++------ server/router/api.go | 2 +- 4 files changed, 123 insertions(+), 159 deletions(-) diff --git a/cmd/server/docs/docs.go b/cmd/server/docs/docs.go index 3e68e31a54..ffd4108519 100644 --- a/cmd/server/docs/docs.go +++ b/cmd/server/docs/docs.go @@ -2242,15 +2242,17 @@ const docTemplate = `{ } } } - }, - "delete": { + } + }, + "/repos/{repo_id}/pipelines/{number}": { + "get": { "produces": [ - "text/plain" + "application/json" ], "tags": [ "Pipelines" ], - "summary": "Delete pipelines", + "summary": "Pipeline information by number", "parameters": [ { "type": "string", @@ -2269,47 +2271,30 @@ const docTemplate = `{ }, { "type": "integer", - "default": 1, - "description": "for response pagination, page offset number", - "name": "page", - "in": "query" - }, - { - "type": "integer", - "default": 50, - "description": "for response pagination, max items per page", - "name": "perPage", - "in": "query" - }, - { - "type": "string", - "description": "only return pipelines before this RFC3339 date", - "name": "before", - "in": "query" - }, - { - "type": "string", - "description": "only return pipelines after this RFC3339 date", - "name": "after", - "in": "query" + "description": "the number of the pipeline, OR 'latest'", + "name": "number", + "in": "path", + "required": true } ], "responses": { - "204": { - "description": "No Content" + "200": { + "description": "OK", + "schema": { + "$ref": "#/definitions/Pipeline" + } } } - } - }, - "/repos/{repo_id}/pipelines/{number}": { - "get": { + }, + "post": { + "description": "Restarts a pipeline optional with altered event, deploy or environment", "produces": [ "application/json" ], "tags": [ "Pipelines" ], - "summary": "Pipeline information by number", + "summary": "Restart a pipeline", "parameters": [ { "type": "string", @@ -2328,10 +2313,22 @@ const docTemplate = `{ }, { "type": "integer", - "description": "the number of the pipeline, OR 'latest'", + "description": "the number of the pipeline", "name": "number", "in": "path", "required": true + }, + { + "type": "string", + "description": "override the event type", + "name": "event", + "in": "query" + }, + { + "type": "string", + "description": "override the target deploy value", + "name": "deploy_to", + "in": "query" } ], "responses": { @@ -2343,15 +2340,14 @@ const docTemplate = `{ } } }, - "post": { - "description": "Restarts a pipeline optional with altered event, deploy or environment", + "delete": { "produces": [ - "application/json" + "text/plain" ], "tags": [ "Pipelines" ], - "summary": "Restart a pipeline", + "summary": "Delete pipeline", "parameters": [ { "type": "string", @@ -2374,26 +2370,11 @@ const docTemplate = `{ "name": "number", "in": "path", "required": true - }, - { - "type": "string", - "description": "override the event type", - "name": "event", - "in": "query" - }, - { - "type": "string", - "description": "override the target deploy value", - "name": "deploy_to", - "in": "query" } ], "responses": { - "200": { - "description": "OK", - "schema": { - "$ref": "#/definitions/Pipeline" - } + "204": { + "description": "No Content" } } } @@ -4533,45 +4514,6 @@ const docTemplate = `{ "EventManual" ] }, - "errors.PipelineError": { - "type": "object", - "properties": { - "data": {}, - "is_warning": { - "type": "boolean" - }, - "message": { - "type": "string" - }, - "type": { - "$ref": "#/definitions/errors.PipelineErrorType" - } - } - }, - "errors.PipelineErrorType": { - "type": "string", - "enum": [ - "linter", - "deprecation", - "compiler", - "generic", - "bad_habit" - ], - "x-enum-comments": { - "PipelineErrorTypeBadHabit": "some bad-habit error", - "PipelineErrorTypeCompiler": "some error with the config semantics", - "PipelineErrorTypeDeprecation": "using some deprecated feature", - "PipelineErrorTypeGeneric": "some generic error", - "PipelineErrorTypeLinter": "some error with the config syntax" - }, - "x-enum-varnames": [ - "PipelineErrorTypeLinter", - "PipelineErrorTypeDeprecation", - "PipelineErrorTypeCompiler", - "PipelineErrorTypeGeneric", - "PipelineErrorTypeBadHabit" - ] - }, "model.Workflow": { "type": "object", "properties": { diff --git a/server/api/pipeline.go b/server/api/pipeline.go index bee44b4e3c..14dcf929f4 100644 --- a/server/api/pipeline.go +++ b/server/api/pipeline.go @@ -121,7 +121,7 @@ func GetPipelines(c *gin.Context) { if before != "" { beforeDt, err := time.Parse(time.RFC3339, before) if err != nil { - _ = c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusBadRequest, err) return } filter.Before = beforeDt.Unix() @@ -130,7 +130,7 @@ func GetPipelines(c *gin.Context) { if after != "" { afterDt, err := time.Parse(time.RFC3339, after) if err != nil { - _ = c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusBadRequest, err) return } filter.After = afterDt.Unix() @@ -144,61 +144,38 @@ func GetPipelines(c *gin.Context) { c.JSON(http.StatusOK, pipelines) } -// DeletePipelines +// DeletePipeline // -// @Summary Delete pipelines -// @Router /repos/{repo_id}/pipelines [delete] +// @Summary Delete pipeline +// @Router /repos/{repo_id}/pipelines/{number} [delete] // @Produce plain // @Success 204 // @Tags Pipelines // @Param Authorization header string true "Insert your personal access token" default(Bearer ) // @Param repo_id path int true "the repository id" -// @Param page query int false "for response pagination, page offset number" default(1) -// @Param perPage query int false "for response pagination, max items per page" default(50) -// @Param before query string false "only return pipelines before this RFC3339 date" -// @Param after query string false "only return pipelines after this RFC3339 date" -func DeletePipelines(c *gin.Context) { - repo := session.Repo(c) - before := c.Query("before") - after := c.Query("after") - - filter := new(model.PipelineFilter) - - if before != "" { - beforeDt, err := time.Parse(time.RFC3339, before) - if err != nil { - _ = c.AbortWithError(http.StatusInternalServerError, err) - return - } - filter.Before = beforeDt.Unix() - } +// @Param number path int true "the number of the pipeline" +func DeletePipeline(c *gin.Context) { + _store := store.FromContext(c) - if after != "" { - afterDt, err := time.Parse(time.RFC3339, after) - if err != nil { - _ = c.AbortWithError(http.StatusInternalServerError, err) - return - } - filter.After = afterDt.Unix() + repo := session.Repo(c) + num, err := strconv.ParseInt(c.Param("number"), 10, 64) + if err != nil { + _ = c.AbortWithError(http.StatusBadRequest, err) + return } - pipelines, err := store.FromContext(c).GetPipelineList(repo, session.Pagination(c), filter) + pl, err := _store.GetPipelineNumber(repo, num) if err != nil { - _ = c.AbortWithError(http.StatusInternalServerError, err) + handleDBError(c, err) return } - for _, pl := range pipelines { - if ok, status := pipelineDeleteAllowed(pl); !ok { - c.String(http.StatusUnprocessableEntity, "Cannot delete pipeline with status %s", status) - return - } - - if delWErr := store.FromContext(c).DeletePipeline(pl); err != nil { - err = errors.Join(err, delWErr) - } + if ok, status := pipelineDeleteAllowed(pl); !ok { + c.String(http.StatusUnprocessableEntity, "Cannot delete pipeline with status %s", status) + return } + err = store.FromContext(c).DeletePipeline(pl) if err != nil { c.String(http.StatusInternalServerError, "Error deleting pipelines. %s", err) return diff --git a/server/api/pipeline_test.go b/server/api/pipeline_test.go index 17473d009a..2d88059c29 100644 --- a/server/api/pipeline_test.go +++ b/server/api/pipeline_test.go @@ -21,39 +21,53 @@ var fakePipeline = &model.Pipeline{ func TestGetPipelines(t *testing.T) { gin.SetMode(gin.TestMode) - pipeline1 := &model.Pipeline{ - Status: model.StatusSuccess, - } - pipeline2 := &model.Pipeline{ - Status: model.StatusPending, - } - g := goblin.Goblin(t) g.Describe("Pipeline", func() { + g.It("should get pipelines", func() { + pipelines := []*model.Pipeline{fakePipeline} + + mockStore := mocks.NewStore(t) + mockStore.On("GetPipelineList", mock.Anything, mock.Anything, mock.Anything).Return(pipelines, nil) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Set("store", mockStore) + + GetPipelines(c) + + mockStore.AssertCalled(t, "GetPipelineList", mock.Anything, mock.Anything, mock.Anything) + assert.Equal(t, http.StatusOK, c.Writer.Status()) + }) + + g.It("should not parse pipeline filter", func() { + c, _ := gin.CreateTestContext(httptest.NewRecorder()) + c.Request, _ = http.NewRequest("DELETE", "/?before=2023-01-16&after=2023-01-15", nil) + + GetPipelines(c) + + assert.Equal(t, http.StatusBadRequest, c.Writer.Status()) + }) + g.It("should parse pipeline filter", func() { - pipelines := make([]*model.Pipeline, 0) - pipelines = append(pipelines, pipeline1) + pipelines := []*model.Pipeline{fakePipeline} mockStore := mocks.NewStore(t) mockStore.On("GetPipelineList", mock.Anything, mock.Anything, mock.Anything).Return(pipelines, nil) - mockStore.On("DeletePipeline", mock.Anything).Return(nil) c, _ := gin.CreateTestContext(httptest.NewRecorder()) c.Set("store", mockStore) - c.Request, _ = http.NewRequest("DELETE", "/?before=2023-01-16T15:00:00Z&after=2023-01-15T15:00:00Z", nil) + c.Request, _ = http.NewRequest("DELETE", "/?2023-01-16T15:00:00Z&after=2023-01-15T15:00:00Z", nil) - DeletePipelines(c) + GetPipelines(c) - assert.Equal(t, http.StatusNoContent, c.Writer.Status()) + assert.Equal(t, http.StatusOK, c.Writer.Status()) }) g.It("should parse pipeline filter with tz offset", func() { - pipelines := make([]*model.Pipeline, 0) - pipelines = append(pipelines, pipeline1) + pipelines := []*model.Pipeline{fakePipeline} mockStore := mocks.NewStore(t) mockStore.On("GetPipelineList", mock.Anything, mock.Anything, mock.Anything).Return(pipelines, nil) - mockStore.On("DeletePipeline", mock.Anything).Return(nil) c, _ := gin.CreateTestContext(httptest.NewRecorder()) c.Set("store", mockStore) @@ -63,20 +77,51 @@ func TestGetPipelines(t *testing.T) { assert.Equal(t, http.StatusOK, c.Writer.Status()) }) + }) +} + +func TestDeletePipeline(t *testing.T) { + gin.SetMode(gin.TestMode) + + g := goblin.Goblin(t) + g.Describe("Pipeline", func() { + g.It("should delete pipeline", func() { + mockStore := mocks.NewStore(t) + mockStore.On("GetPipelineNumber", mock.Anything, mock.Anything).Return(fakePipeline, nil) + mockStore.On("DeletePipeline", mock.Anything).Return(nil) + + c, _ := gin.CreateTestContext(httptest.NewRecorder()) + c.Set("store", mockStore) + c.Params = gin.Params{{Key: "number", Value: "1"}} + + DeletePipeline(c) + + mockStore.AssertCalled(t, "GetPipelineNumber", mock.Anything, mock.Anything) + mockStore.AssertCalled(t, "DeletePipeline", mock.Anything) + assert.Equal(t, http.StatusNoContent, c.Writer.Status()) + }) + + g.It("should not delete without pipeline number", func() { + c, _ := gin.CreateTestContext(httptest.NewRecorder()) + + DeletePipeline(c) + + assert.Equal(t, http.StatusBadRequest, c.Writer.Status()) + }) g.It("should not delete pending", func() { - pipelines := make([]*model.Pipeline, 0) - pipelines = append(pipelines, pipeline2) + fakePipeline.Status = model.StatusPending mockStore := mocks.NewStore(t) - mockStore.On("GetPipelineList", mock.Anything, mock.Anything, mock.Anything).Return(pipelines, nil) + mockStore.On("GetPipelineNumber", mock.Anything, mock.Anything).Return(fakePipeline, nil) c, _ := gin.CreateTestContext(httptest.NewRecorder()) c.Set("store", mockStore) + c.Params = gin.Params{{Key: "number", Value: "1"}} - DeletePipelines(c) + DeletePipeline(c) - mockStore.AssertCalled(t, "GetPipelineList", mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertCalled(t, "GetPipelineNumber", mock.Anything, mock.Anything) mockStore.AssertNotCalled(t, "DeletePipeline", mock.Anything) assert.Equal(t, http.StatusUnprocessableEntity, c.Writer.Status()) }) diff --git a/server/router/api.go b/server/router/api.go index dd3505d826..80453b320e 100644 --- a/server/router/api.go +++ b/server/router/api.go @@ -94,7 +94,7 @@ func apiRoutes(e *gin.RouterGroup) { repo.GET("/pipelines", api.GetPipelines) repo.POST("/pipelines", session.MustPush, api.CreatePipeline) - repo.DELETE("/pipelines", session.MustRepoAdmin(), api.DeletePipelines) + repo.DELETE("/pipelines/:number", session.MustRepoAdmin(), api.DeletePipeline) repo.GET("/pipelines/:number", api.GetPipeline) repo.GET("/pipelines/:number/config", api.GetPipelineConfig) From a4bc6e2b552e0aeac23b6eb8837b9e3d8175937c Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Thu, 25 Apr 2024 10:21:01 +0200 Subject: [PATCH 13/14] dont return known pipeline status --- server/api/helper.go | 6 +++--- server/api/pipeline.go | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/server/api/helper.go b/server/api/helper.go index 12b52efa86..dbe6ce6e2e 100644 --- a/server/api/helper.go +++ b/server/api/helper.go @@ -67,11 +67,11 @@ func refreshUserToken(c *gin.Context, user *model.User) { // pipelineDeleteAllowed checks if the given pipeline can be deleted based on its status. // It returns a bool indicating if delete is allowed, and the pipeline's status. -func pipelineDeleteAllowed(pl *model.Pipeline) (bool, model.StatusValue) { +func pipelineDeleteAllowed(pl *model.Pipeline) bool { switch pl.Status { case model.StatusRunning, model.StatusPending, model.StatusBlocked: - return false, pl.Status + return false } - return true, pl.Status + return true } diff --git a/server/api/pipeline.go b/server/api/pipeline.go index 14dcf929f4..5114ed8b95 100644 --- a/server/api/pipeline.go +++ b/server/api/pipeline.go @@ -170,8 +170,8 @@ func DeletePipeline(c *gin.Context) { return } - if ok, status := pipelineDeleteAllowed(pl); !ok { - c.String(http.StatusUnprocessableEntity, "Cannot delete pipeline with status %s", status) + if ok := pipelineDeleteAllowed(pl); !ok { + c.String(http.StatusUnprocessableEntity, "Cannot delete pipeline with status %s", pl.Status) return } @@ -614,8 +614,8 @@ func DeletePipelineLogs(c *gin.Context) { return } - if ok, status := pipelineDeleteAllowed(pl); !ok { - c.String(http.StatusUnprocessableEntity, "Cannot delete logs for pipeline with status %s", status) + if ok := pipelineDeleteAllowed(pl); !ok { + c.String(http.StatusUnprocessableEntity, "Cannot delete logs for pipeline with status %s", pl.Status) return } From 9d09d7c44bd81d7ee13487a34abde7bef7cb7307 Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Thu, 25 Apr 2024 10:30:29 +0200 Subject: [PATCH 14/14] fix spelling in error message --- server/api/pipeline.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/api/pipeline.go b/server/api/pipeline.go index 5114ed8b95..b7af599560 100644 --- a/server/api/pipeline.go +++ b/server/api/pipeline.go @@ -177,7 +177,7 @@ func DeletePipeline(c *gin.Context) { err = store.FromContext(c).DeletePipeline(pl) if err != nil { - c.String(http.StatusInternalServerError, "Error deleting pipelines. %s", err) + c.String(http.StatusInternalServerError, "Error deleting pipeline. %s", err) return }