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.

Sign up for GitHub

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

Jump to bottom

refactor: encryption refactor #2429

Merged
merged 2 commits into from
Aug 19, 2024
Merged

refactor: encryption refactor #2429

merged 2 commits into from
Aug 19, 2024

Conversation

gak
Copy link
Contributor

@gak gak commented Aug 19, 2024

Fixes #2346

@gak gak requested a review from alecthomas as a code owner August 19, 2024 05:38
@gak gak requested review from a team and stuartwdouglas and removed request for a team August 19, 2024 05:38
@github-actions github-actions bot changed the title Encryption refactor refactor: encryption refactor Aug 19, 2024
@ftl-robot ftl-robot mentioned this pull request Aug 19, 2024
Open
@gak gak added the run-all A PR with this label will run the full set of CI jobs in the PR rather than in the merge queue label Aug 19, 2024
@gak gak force-pushed the gak/encryption-refactor branch from 396a585 to fbaeb39 Compare August 19, 2024 06:58
alecthomas
alecthomas reviewed Aug 19, 2024
View reviewed changes
Copy link
Collaborator

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Nice! Unfortunately this is going to conflict with #2400 from Friday, but not much we can do about it now 🙁

backend/controller/cronjobs/cronjobs_test.go Outdated Show resolved Hide resolved
backend/controller/dal/async_calls.go Outdated Show resolved Hide resolved
backend/controller/dal/encryption.go Outdated Show resolved Hide resolved
internal/encryption/encryption.go Outdated Show resolved Hide resolved
internal/encryption/encryption.go Outdated Show resolved Hide resolved
internal/encryption/encryption.go Outdated Show resolved Hide resolved
@gak gak force-pushed the gak/encryption-refactor branch from c1c8333 to a01b7dc Compare August 19, 2024 20:08
@gak
wip
1967918 
chore: refactor encryption so dal does not know about awscli

chore: lint

chore(autofmt): Automated formatting

fix: integration

fix: immutable receivers, cleanups

refactor: rename things to make the next merge easier
@gak gak force-pushed the gak/encryption-refactor branch from a01b7dc to 1967918 Compare August 19, 2024 20:59
alecthomas
alecthomas approved these changes Aug 19, 2024
View reviewed changes
internal/encryption/encryption.go
// EnsureKey asks a provider to check for an encrypted key.
// If not available, call the generateKey function to create a new key.
// The provider should handle transactions around checking and setting the key, to prevent race conditions.
EnsureKey(ctx context.Context, generateKey func() ([]byte, error)) ([]byte, error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice.

@alecthomas
Copy link
Collaborator

Awesome, this feels pretty clean!

@alecthomas alecthomas added this pull request to the merge queue Aug 19, 2024
Merged via the queue into main with commit 229e3d5 Aug 19, 2024
74 checks passed
@alecthomas alecthomas deleted the gak/encryption-refactor branch August 19, 2024 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alecthomas alecthomas alecthomas approved these changes

@stuartwdouglas stuartwdouglas Awaiting requested review from stuartwdouglas

Assignees
No one assigned
Labels
run-all A PR with this label will run the full set of CI jobs in the PR rather than in the merge queue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Encryption provider
2 participants
@gak @alecthomas