Skip to content

Conversation

@jbrockopp
Copy link
Contributor

@jbrockopp jbrockopp commented Apr 8, 2023

Based off of #574, #663, #687, #692, #721, #722 and #782

This change continues the refactor efforts initially introduced in the above PRs.

This adds a new step package to the github.com/go-vela/server/database package.

This contains a StepService interface declaring all functions necessary for step based interactions with the database:

// StepService represents the Vela interface for step
// functions with the supported Database backends.
//
//nolint:revive // ignore name stutter
type StepService interface {
// Step Data Definition Language Functions
//
// https://en.wikipedia.org/wiki/Data_definition_language
// CreateStepTable defines a function that creates the steps table.
CreateStepTable(string) error
// Step Data Manipulation Language Functions
//
// https://en.wikipedia.org/wiki/Data_manipulation_language
// CountSteps defines a function that gets the count of all steps.
CountSteps() (int64, error)
// CountStepsForBuild defines a function that gets the count of steps by build ID.
CountStepsForBuild(*library.Build, map[string]interface{}) (int64, error)
// CreateStep defines a function that creates a new step.
CreateStep(*library.Step) error
// DeleteStep defines a function that deletes an existing step.
DeleteStep(*library.Step) error
// GetStep defines a function that gets a step by ID.
GetStep(int64) (*library.Step, error)
// GetStepForBuild defines a function that gets a step by number and build ID.
GetStepForBuild(*library.Build, int) (*library.Step, error)
// ListSteps defines a function that gets a list of all steps.
ListSteps() ([]*library.Step, error)
// ListStepsForBuild defines a function that gets a list of steps by build ID.
ListStepsForBuild(*library.Build, map[string]interface{}, int, int) ([]*library.Step, int64, error)
// ListStepImageCount defines a function that gets a list of all step images and the count of their occurrence.
ListStepImageCount() (map[string]float64, error)
// ListStepStatusCount defines a function that gets a list of all step statuses and the count of their occurrence.
ListStepStatusCount() (map[string]float64, error)
// UpdateStep defines a function that updates an existing step.
UpdateStep(*library.Step) error
}

This package also contains the engine which implements the above service interface:

// engine represents the step functionality that implements the StepService interface.
engine struct {
// engine configuration settings used in step functions
config *config
// gorm.io/gorm database client used in step functions
//
// https://pkg.go.dev/gorm.io/gorm#DB
client *gorm.DB
// sirupsen/logrus logger used in step functions
//
// https://pkg.go.dev/github.com/sirupsen/logrus#Entry
logger *logrus.Entry
}
)

This engine contains no raw SQL queries for integrating with the steps table.

Instead, we leverage our DB library's (https://gorm.io/) agnostic abstraction for integrating with that table.

@jbrockopp jbrockopp added the enhancement Indicates an improvement to a feature label Apr 8, 2023
@jbrockopp jbrockopp self-assigned this Apr 8, 2023
@codecov
Copy link

codecov bot commented Apr 8, 2023

Codecov Report

Merging #810 (fadf283) into main (05371c1) will decrease coverage by 0.22%.
The diff coverage is 81.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #810      +/-   ##
==========================================
- Coverage   58.12%   57.91%   -0.22%     
==========================================
  Files         255      263       +8     
  Lines       15892    15862      -30     
==========================================
- Hits         9238     9187      -51     
- Misses       6249     6261      +12     
- Partials      405      414       +9     
Impacted Files Coverage Δ
api/build.go 1.48% <0.00%> (ø)
api/metrics.go 0.00% <0.00%> (ø)
api/step.go 0.00% <0.00%> (ø)
database/step/step.go 56.00% <56.00%> (ø)
database/sqlite/sqlite.go 69.49% <62.50%> (+0.52%) ⬆️
database/postgres/postgres.go 71.26% <70.00%> (+0.67%) ⬆️
database/step/list.go 72.72% <72.72%> (ø)
database/step/list_build.go 79.06% <79.06%> (ø)
database/step/get.go 81.25% <81.25%> (ø)
database/step/get_build.go 85.00% <85.00%> (ø)
... and 10 more

@jbrockopp jbrockopp marked this pull request as ready for review April 11, 2023 14:32
@jbrockopp jbrockopp requested a review from a team as a code owner April 11, 2023 14:32
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few nits.

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits resolved. No changes needed. LGTM!

Thanks again for doing all this refactoring. The database and API layers are feeling much cleaner.

Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super minor copy pasta error, but i don't want to hold back merge


// create the database agnostic step service
//
// https://pkg.go.dev/github.com/go-vela/server/database/repo#New
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// https://pkg.go.dev/github.com/go-vela/server/database/repo#New
// https://pkg.go.dev/github.com/go-vela/server/database/step#New

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch 😄 I've addressed this within 19c554e as a part of #816

My line of thinking is we can merge this one as-is and that PR will resolve the typo

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

Labels

enhancement Indicates an improvement to a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants