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

pgsql #51

Merged
merged 25 commits into from
Jul 15, 2022
Merged

pgsql #51

merged 25 commits into from
Jul 15, 2022

Conversation

sheshenia
Copy link
Contributor

Added PostgreSQL storage.
All conflicts solved. Integration tests pass ok.
Enrolled a real device, tested with cmdr.py commands - ok. Compared the results in multi-storage mode MySQL - PostgreSQL. Data in both DB tables are the same.
integration_test_pgsql

@jessepeterson jessepeterson self-requested a review July 15, 2022 17:19
Copy link
Member

@jessepeterson jessepeterson left a comment

Choose a reason for hiding this comment

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

This is great! Just a couple really minor things this time 'round. Also I was able to throw a PostgreSQL instance up and use the nano2nano tool to migrate my enrollments to it and it all seems to be working great on very initial poking at it!

@@ -1,4 +1,4 @@
// Pacakge mysql stores and retrieves MDM data from SQL
// Package mysql Package mysql stores and retrieves MDM data from MySQL
Copy link
Member

Choose a reason for hiding this comment

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

Duplicated 'package mysql'?

storage/mysql/mysql.go Outdated Show resolved Hide resolved
r.Context, `
INSERT INTO cert_auth_associations (id, sha256)
VALUES ($1, $2)
ON CONFLICT ON CONSTRAINT cert_auth_associations_pkey DO NOTHING;`,
Copy link
Member

Choose a reason for hiding this comment

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

Nice! I see you added this from your comment from #50.

Copy link
Member

Choose a reason for hiding this comment

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

On second thought there is one reason why you might not want to DO NOTHING. And that's to update the timestamp. Does the timestamp update on a do nothing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated_at time doesn't update on "do nothing". The solution is
ON CONFLICT ON CONSTRAINT cert_auth_associations_pkey DO UPDATE SET updated_at=now();
Should I make changes?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, go for it!

}

// previous: `SELECT id, topic, push_magic, token_hex FROM enrollments WHERE id IN (`+qs+`);`,
// refactor all strings concatenations with strings.Builder which is more efficient
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,111 @@
//go:build integration
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Co-authored-by: Jesse Peterson <jessepeterson@users.noreply.github.com>
Copy link
Member

@jessepeterson jessepeterson left a comment

Choose a reason for hiding this comment

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

lgtm! note to others the previous review with comments was under #50.

@jessepeterson jessepeterson merged commit 9ca3cac into micromdm:main Jul 15, 2022
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.

2 participants