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

feat(db): add repository package for persistence #170

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

glimchb
Copy link
Member

@glimchb glimchb commented Sep 8, 2023

This is not full implementation yet, just the start

Signed-off-by: Boris Glimcher Boris.Glimcher@emc.com

Copy link

@pwalessi-dell pwalessi-dell left a comment

Choose a reason for hiding this comment

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

I think that this is good! Just a couple of comments, no biggies though.

}

// Factory pattern to create new Database
func Factory(databaseImplementation string) (Database, error) {

Choose a reason for hiding this comment

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

You probably want to define a database enum instead of using a string

Choose a reason for hiding this comment

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

For databases that are backed by a provided service, i.e. Redis (or anything not in memory or a persistent volume), you will need to pass the url, host, port, user, pass, etc... I think it would be worth creating a config struct in the repository package for these values and passing that to the factory too. All keys would be optional and you would rely on code documentation to state which keys are required for each implementation.

You could include the databaseImplementation value as part of this config struct too.

Depending on the flexibility required, you could create a config struct based on a free-form map[string]string. This wouldn't force you to pre-define the keys required for a Database instance, but instead you would rely on database implementations clearly documenting the required key-value config entries required. An example of this is here: https://matthewbrown.io/2016/01/23/factory-pattern-in-golang

package repository

// Database abstraction
type Database interface {

Choose a reason for hiding this comment

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

You might want to call it KVStore or something like that. It's not really a "database".

Choose a reason for hiding this comment

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

You might also consider including mocked implementations of this interface to encourage that testing pattern.

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #170 (8233dbe) into main (71379e5) will decrease coverage by 1.13%.
Report is 6 commits behind head on main.
The diff coverage is 51.28%.

@@            Coverage Diff             @@
##             main     #170      +/-   ##
==========================================
- Coverage   76.12%   75.00%   -1.13%     
==========================================
  Files           5        5              
  Lines        1001     1012      +11     
==========================================
- Hits          762      759       -3     
- Misses        210      218       +8     
- Partials       29       35       +6     
Files Changed Coverage Δ
pkg/evpn/bridge.go 73.76% <42.85%> (-1.99%) ⬇️
pkg/evpn/svi.go 73.54% <42.85%> (-1.56%) ⬇️
pkg/evpn/vrf.go 75.95% <42.85%> (-1.53%) ⬇️
pkg/evpn/port.go 74.13% <55.55%> (-1.52%) ⬇️
pkg/evpn/evpn.go 84.74% <66.66%> (+6.74%) ⬆️

pkg/repository/models.go Fixed Show fixed Hide fixed
pkg/repository/models.go Fixed Show fixed Hide fixed
pkg/repository/models.go Fixed Show fixed Hide fixed
pkg/repository/redis.go Fixed Show fixed Hide fixed
cmd/main.go Outdated
@@ -28,6 +29,12 @@ func main() {
if err != nil {
log.Fatalf("failed to listen: %v", err)
}
db, err := repository.Factory("memory")

Choose a reason for hiding this comment

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

I assume the Database interface (or db instance here) would be passed to subsequent services, right? Allowing you to pass a mock database to those services during testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

correct

This is not full implementation yet, just the start

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
@@ -0,0 +1,38 @@
// SPDX-License-Identifier: Apache-2.0
// Copyright (c) 2022-2023 Dell Inc, or its subsidiaries.
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to a separate PR #267

// SPDX-License-Identifier: Apache-2.0
// Copyright (c) 2022-2023 Dell Inc, or its subsidiaries.

// Package repository is the database abstraction implementing repository design pattern
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to a separate PR #267

@glimchb glimchb added the invalid This doesn't seem right label Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(store): use gokv pkg to abstract persistant store
3 participants