-
Notifications
You must be signed in to change notification settings - Fork 30
refactor(database): move secret logic into separate package #782
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
Conversation
…to refactor/database/repo
…to refactor/database/worker
cognifloyd
left a comment
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.
This is so much cleaner! Thanks for working on this.
I started reviewing and have a few comments/requests about handling all secret types.
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.
On the count methods, the filters arg looks like a new feature. Where/when might this be used?
Also, where are we planning to use the count all secrets (database I don't see anything that uses it in the native secrets engine.CountSecrets) method?
edit: Oh. I see CountSecrets is used by the ListSecrets method. And that is probably useful for migrations or to rotate the encryption key or similar.
cognifloyd
left a comment
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.
Ok. I finished my first pass through this PR. This feels a lot cleaner.
I just noticed a couple bits of copy pasta to clean up.
cognifloyd
left a comment
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.
Awesome. Thank you!
plyr4
left a comment
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.
extremely minor comment typos
…into refactor/database/secret
ecrupper
left a comment
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.
Nice — LGTM
Based off of #574, #663, #687, #692 #721 and #722
This change continues the refactor efforts initially introduced in the above PRs.
This adds a new
secretpackage to thegithub.amrom.workers.dev/go-vela/server/databasepackage.This contains a
SecretServiceinterface declaring all functions necessary for worker based interactions with the database:server/database/secret/service.go
Lines 11 to 63 in 662917e
This package also contains the
enginewhich implements the above service interface:server/database/secret/secret.go
Lines 25 to 39 in 662917e
This
enginecontains no raw SQL queries for integrating with thesecretstable.Instead, we leverage our DB library's (https://gorm.io/) agnostic abstraction for integrating with that table.