Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(database): use agnostic engine #873

Merged
merged 35 commits into from
Jun 8, 2023
Merged

Conversation

jbrockopp
Copy link
Contributor

@jbrockopp jbrockopp commented Jun 6, 2023

Dependent on #870

This change updates the database.New() function to return the database agnostic engine{} type introduced in #868.

The logic for this function was copied from the duplicate code we already have in both supported drivers:

This also adds a new FromCLIContext() function to the database package to create the agnostic engine{}.

jbrockopp and others added 30 commits February 14, 2023 14:00
@jbrockopp jbrockopp added the enhancement Indicates an improvement to a feature label Jun 6, 2023
@jbrockopp jbrockopp self-assigned this Jun 6, 2023
@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #873 (d5594e9) into main (da73438) will decrease coverage by 0.25%.
The diff coverage is 65.15%.

❗ Current head d5594e9 differs from pull request most recent head f352077. Consider uploading reports for the commit f352077 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #873      +/-   ##
==========================================
- Coverage   72.07%   71.82%   -0.25%     
==========================================
  Files         312      311       -1     
  Lines       13049    13026      -23     
==========================================
- Hits         9405     9356      -49     
- Misses       3188     3209      +21     
- Partials      456      461       +5     
Impacted Files Coverage Δ
database/database.go 62.29% <56.60%> (-37.71%) ⬇️
database/context.go 88.00% <100.00%> (-12.00%) ⬇️

... and 2 files with indirect coverage changes

)

// helper function to setup the database from the CLI arguments.
func setupDatabase(c *cli.Context) (database.Interface, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic was moved to FromCLIContext() function inside the database package.

@@ -35,3 +38,19 @@ func FromContext(c context.Context) Interface {
func ToContext(c Setter, d Interface) {
c.Set(key, d)
}

// FromCLIContext creates and returns a database engine from the urfave/cli context.
func FromCLIContext(c *cli.Context) (Interface, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic was copied from the setupDatabase() function in the cmd/vela-server/database.go file.


"github.com/gin-gonic/gin"
"github.com/go-vela/server/database/sqlite"
"github.com/urfave/cli/v2"
)

func TestDatabase_FromContext(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was refactored to use table driven tests.

func TestDatabase_FromContext_WrongType(t *testing.T) {
// setup context
gin.SetMode(gin.TestMode)
func TestDatabase_ToContext(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was refactored to use table driven tests.

// NewTest creates and returns an engine that integrates with an in-memory database provider.
//
// This function is ONLY intended to be used for testing purposes.
func NewTest() (Interface, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be needed for tests all over the place that have a dependency on database logic

i.e. middleware, secret etc.

// TODO: Add this function to the interface once other code has been updated to use the agnostic engine.
//
// Close defines a function that stops and terminates the connection to the database.
// Close() error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't add this function to the interface until we update all code to call database.NewTest().

That will be added in a separate PR because it will touch a lot of code.

@jbrockopp jbrockopp marked this pull request as ready for review June 7, 2023 17:04
@jbrockopp jbrockopp requested a review from a team as a code owner June 7, 2023 17:04
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.

3 participants