-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add MySQL storage and tests #5
Changes from 16 commits
cd04701
e5554fb
37aed61
4aafd64
fa61974
15a774b
f0dd734
1c988d7
478cc3e
2ef1eca
ea941b4
31fced2
95a8f2b
0a82006
21464cb
bb4cb0f
1bb90ee
465df8b
63fe581
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
package parse | ||
|
||
import ( | ||
"fmt" | ||
|
||
_ "github.com/go-sql-driver/mysql" | ||
"github.com/micromdm/nanodep/storage" | ||
"github.com/micromdm/nanodep/storage/file" | ||
"github.com/micromdm/nanodep/storage/mysql" | ||
) | ||
|
||
// Storage parses a storage name and dsn to determine which and return a storage backend. | ||
func Storage(storageName, dsn string) (storage.AllStorage, error) { | ||
var store storage.AllStorage | ||
var err error | ||
switch storageName { | ||
case "file": | ||
if dsn == "" { | ||
dsn = "db" | ||
} | ||
store, err = file.New(dsn) | ||
case "mysql": | ||
lucasmrod marked this conversation as resolved.
Show resolved
Hide resolved
|
||
store, err = mysql.New(mysql.WithDSN(dsn)) | ||
default: | ||
return nil, fmt.Errorf("unknown storage: %q", storageName) | ||
} | ||
return store, err | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ import ( | |
"time" | ||
|
||
"github.com/micromdm/nanodep/client" | ||
"github.com/micromdm/nanodep/storage" | ||
) | ||
|
||
const defaultFileMode = 0644 | ||
|
@@ -20,6 +21,8 @@ type FileStorage struct { | |
path string | ||
} | ||
|
||
var _ storage.AllStorage = (*FileStorage)(nil) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious what's the reason for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Convention that serves both as a "compile time check" and as a form of documentation for code readers/reviewers that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this creates an unnecessary coupling. How do you feel about removing it? As Go is statically typed an implementation must conform to it's use. Let me know what you think. :) |
||
|
||
// New creates a new FileStorage backend. | ||
func New(path string) (*FileStorage, error) { | ||
err := os.Mkdir(path, 0755) | ||
|
@@ -62,10 +65,17 @@ func (s *FileStorage) tokenpkiFilename(name, kind string) string { | |
// RetrieveAuthTokens reads the JSON DEP OAuth tokens from disk for name DEP name. | ||
func (s *FileStorage) RetrieveAuthTokens(_ context.Context, name string) (*client.OAuth1Tokens, error) { | ||
tokens := new(client.OAuth1Tokens) | ||
return tokens, decodeJSONfile(s.tokensFilename(name), tokens) | ||
err := decodeJSONfile(s.tokensFilename(name), tokens) | ||
if err != nil { | ||
if errors.Is(err, os.ErrNotExist) { | ||
return nil, storage.ErrNotFound | ||
} | ||
return nil, err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any problem with returning a non-nil empty token — most callers are going to check for errors anyway? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A convention I've seen many projects use is to try (when possible) to always return zero-values when error is non-nil. In this case, the zero-value of a pointer is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I'm fine with this specific code change, although its a bit verbose. But in general I don't think its necessary to add a bunch of boilerplate to assure a zero-values for an error. Discussed here: https://macadmins.slack.com/archives/C07ME1284/p1663867171949419 |
||
} | ||
return tokens, nil | ||
} | ||
|
||
// RetrieveAuthTokens saves the DEP OAuth tokens to disk as JSON for name DEP name. | ||
// StoreAuthTokens saves the DEP OAuth tokens to disk as JSON for name DEP name. | ||
func (s *FileStorage) StoreAuthTokens(_ context.Context, name string, tokens *client.OAuth1Tokens) error { | ||
f, err := os.Create(s.tokensFilename(name)) | ||
if err != nil { | ||
|
@@ -85,8 +95,8 @@ func decodeJSONfile(filename string, v interface{}) error { | |
} | ||
|
||
// RetrieveConfig reads the JSON DEP config from disk for name DEP name. | ||
// We return an empty config if the config does not exist on disk. | ||
// This is to support a fallback default config. | ||
// | ||
// Returns an empty config if the config does not exist (to support a fallback default config). | ||
func (s *FileStorage) RetrieveConfig(_ context.Context, name string) (*client.Config, error) { | ||
config := new(client.Config) | ||
err := decodeJSONfile(s.configFilename(name), config) | ||
|
@@ -109,6 +119,8 @@ func (s *FileStorage) StoreConfig(_ context.Context, name string, config *client | |
|
||
// RetrieveAssignerProfile reads the assigner profile UUID and its configured | ||
// timestamp from disk for name DEP name. | ||
// | ||
// Returns an empty profile if it does not exist. | ||
func (s *FileStorage) RetrieveAssignerProfile(_ context.Context, name string) (string, time.Time, error) { | ||
profileBytes, err := os.ReadFile(s.profileFilename(name)) | ||
if err != nil && errors.Is(err, os.ErrNotExist) { | ||
|
@@ -119,7 +131,7 @@ func (s *FileStorage) RetrieveAssignerProfile(_ context.Context, name string) (s | |
if err == nil { | ||
var stat fs.FileInfo | ||
stat, err = os.Stat(s.profileFilename(name)) | ||
if err != nil { | ||
if err == nil { | ||
lucasmrod marked this conversation as resolved.
Show resolved
Hide resolved
|
||
modTime = stat.ModTime() | ||
} | ||
} | ||
|
@@ -164,10 +176,16 @@ func (s *FileStorage) StoreTokenPKI(_ context.Context, name string, pemCert []by | |
func (s *FileStorage) RetrieveTokenPKI(_ context.Context, name string) ([]byte, []byte, error) { | ||
certBytes, err := os.ReadFile(s.tokenpkiFilename(name, "cert")) | ||
if err != nil { | ||
if errors.Is(err, os.ErrNotExist) { | ||
return nil, nil, storage.ErrNotFound | ||
} | ||
return nil, nil, err | ||
} | ||
keyBytes, err := os.ReadFile(s.tokenpkiFilename(name, "key")) | ||
if err != nil { | ||
if errors.Is(err, os.ErrNotExist) { | ||
return nil, nil, storage.ErrNotFound | ||
} | ||
return nil, nil, err | ||
} | ||
return certBytes, keyBytes, err | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of moving this out of the
cmd/
tree. But can we rename this package to justcli
to match micromdm/nanomdm@ff8b7af?