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

test(encryption): add integration test for db encryption #2133

Merged
merged 3 commits into from May 13, 2024

Conversation

bodinsamuel
Copy link
Contributor

@bodinsamuel bodinsamuel commented May 10, 2024

Describe your changes

  • Test encryptDatabaseIfNeeded
    Only check environment because that what I was trying to fix

How to test?

  • Removing NANGO_ENCRYPTION_KEY from your .env should throw Error: A previously set NANGO_ENCRYPTION_KEY has been removed from your environment variables.
  • Changing NANGO_ENCRYPTION_KEY should throw Rotation of NANGO_ENCRYPTION_KEY is not supported.

@bodinsamuel bodinsamuel self-assigned this May 10, 2024
@bodinsamuel bodinsamuel changed the title Fix/test encryption test(encryption): add integration test for db encryption May 10, 2024
@bodinsamuel bodinsamuel marked this pull request as ready for review May 13, 2024 13:07
}

await this.encryptDatabase();
await this.saveDbConfig({ encryption_key_hash: encryptionKeyHash, encryption_complete: true });
Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated to your changes but all the queries in encryptDatabase and saveDbConfig should probably be inside a transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first glance yes, I'm just wondering about the size of the transaction if you have a large db. I think the script assume it can fail at any time and try everything from the start every time which is nice imo

@bodinsamuel bodinsamuel merged commit b107aff into fix/schema-migration May 13, 2024
21 checks passed
@bodinsamuel bodinsamuel deleted the fix/test-encryption branch May 13, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants