Skip to content

Conversation

@jbrockopp
Copy link
Contributor

@jbrockopp jbrockopp commented May 22, 2023

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

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

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

This contains a BuildInterface interface declaring all functions necessary for build based interactions with the database:

// BuildInterface represents the Vela interface for build
// functions with the supported Database backends.
//
//nolint:revive // ignore name stutter
type BuildInterface interface {
// Build Data Definition Language Functions
//
// https://en.wikipedia.org/wiki/Data_definition_language
// CreateBuildIndexes defines a function that creates the indexes for the builds table.
CreateBuildIndexes() error
// CreateBuildTable defines a function that creates the builds table.
CreateBuildTable(string) error
// Build Data Manipulation Language Functions
//
// https://en.wikipedia.org/wiki/Data_manipulation_language
// CountBuilds defines a function that gets the count of all builds.
CountBuilds() (int64, error)
// CountBuildsForDeployment defines a function that gets the count of builds by deployment url.
CountBuildsForDeployment(*library.Deployment, map[string]interface{}) (int64, error)
// CountBuildsForOrg defines a function that gets the count of builds by org name.
CountBuildsForOrg(string, map[string]interface{}) (int64, error)
// CountBuildsForRepo defines a function that gets the count of builds by repo ID.
CountBuildsForRepo(*library.Repo, map[string]interface{}) (int64, error)
// CountBuildsForStatus defines a function that gets the count of builds by status.
CountBuildsForStatus(string, map[string]interface{}) (int64, error)
// CreateBuild defines a function that creates a new build.
CreateBuild(*library.Build) error
// DeleteBuild defines a function that deletes an existing build.
DeleteBuild(*library.Build) error
// GetBuild defines a function that gets a build by ID.
GetBuild(int64) (*library.Build, error)
// GetBuildForRepo defines a function that gets a build by repo ID and number.
GetBuildForRepo(*library.Repo, int) (*library.Build, error)
// LastBuildForRepo defines a function that gets the last build ran by repo ID and branch.
LastBuildForRepo(*library.Repo, string) (*library.Build, error)
// ListBuilds defines a function that gets a list of all builds.
ListBuilds() ([]*library.Build, error)
// ListBuildsForDeployment defines a function that gets a list of builds by deployment url.
ListBuildsForDeployment(*library.Deployment, map[string]interface{}, int, int) ([]*library.Build, int64, error)
// ListBuildsForOrg defines a function that gets a list of builds by org name.
ListBuildsForOrg(string, map[string]interface{}, int, int) ([]*library.Build, int64, error)
// ListBuildsForRepo defines a function that gets a list of builds by repo ID.
ListBuildsForRepo(*library.Repo, map[string]interface{}, int64, int64, int, int) ([]*library.Build, int64, error)
// ListPendingAndRunningBuilds defines a function that gets a list of pending and running builds.
ListPendingAndRunningBuilds(string) ([]*library.BuildQueue, error)
// UpdateBuild defines a function that updates an existing build.
UpdateBuild(*library.Build) error
}

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

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

This engine contains no raw SQL queries for integrating with the builds 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 May 22, 2023
@jbrockopp jbrockopp self-assigned this May 22, 2023
@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

Merging #858 (f00e6fe) into main (5a9a92d) will decrease coverage by 0.68%.
The diff coverage is 79.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #858      +/-   ##
==========================================
- Coverage   66.79%   66.12%   -0.68%     
==========================================
  Files         283      297      +14     
  Lines       14144    13879     -265     
==========================================
- Hits         9448     9177     -271     
- Misses       4249     4255       +6     
  Partials      447      447              
Impacted Files Coverage Δ
api/badge.go 0.00% <0.00%> (ø)
api/build.go 1.34% <0.00%> (ø)
api/metrics.go 0.00% <0.00%> (ø)
api/webhook.go 0.00% <0.00%> (ø)
database/build/index.go 47.05% <47.05%> (ø)
database/build/build.go 51.72% <51.72%> (ø)
database/build/last_repo.go 72.00% <72.00%> (ø)
database/build/list.go 72.72% <72.72%> (ø)
database/sqlite/sqlite.go 74.76% <72.72%> (+5.00%) ⬆️
database/build/list_deployment.go 78.57% <78.57%> (ø)
... and 17 more

@jbrockopp jbrockopp marked this pull request as ready for review May 22, 2023 20:44
@jbrockopp jbrockopp requested a review from a team as a code owner May 22, 2023 20:44
cognifloyd
cognifloyd previously approved these changes May 23, 2023

for _, deployment := range d {
b, err := database.FromContext(c).GetDeploymentBuildList(*deployment.URL)
b, _, err := database.FromContext(c).ListBuildsForDeployment(deployment, nil, 1, 3)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about this line here. Is there a maximum number of three builds that can result from a deployment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My memory is a little fuzzy on the exact details but I believe 3 was chosen as a starting point? 😅

IIRC, we don't have anything that limits the amount of builds that can be tied to a specific deployment 👍

The reason why I'm using 3 here is because that's how the code was originally implemented:

#471

FWIW - I have no objections to increasing this to 10 since that's more consistent with our default per_page.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, interesting... didn't catch that. Yeah I don't really care tbh, 3 works.

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.

4 participants