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

More server DB tests #854

Merged
merged 4 commits into from
Jul 27, 2016
Merged

Conversation

cyli
Copy link
Contributor

@cyli cyli commented Jul 20, 2016

This finishes the refactor in #824 of the server storage tests refactor to take a DB type.

In addition, this fixes:

  1. Deleting a GUN in mysql - previously, we soft-deleted a gun, preventing us from being able to re-use the namespace. We now hard-delete.
  2. The RethinkDB server health check needs to fail if the session user does not have the correct permissions to access the requisite tables.

@cyli cyli force-pushed the more-server-db-test-refactor branch 2 times, most recently from 16af005 to 119dd7c Compare July 20, 2016 01:08
@@ -14,6 +14,7 @@ type TUFFile struct {

// TableName sets a specific table name for TUFFile
func (g TUFFile) TableName() string {
// NOTE: if this value changes, please also change it in SQLStorage.Delete
Copy link
Contributor

@riyazdf riyazdf Jul 20, 2016

Choose a reason for hiding this comment

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

Is it worth making the table names constants in the storage package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately we didn't want to string-interpolate an SQL statement. :| Which means hard-coding it in the DELETE string. Probably the right way to fix this is to submit a patch to GORM upstream to enable hard-delete?

Although there's this scope.Search.Unscoped check in the delete function that I don't really know how to use: https://github.com/jinzhu/gorm/blob/master/callback_delete.go#L29 - maybe that does something interesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that seems to work, thanks!

@riyazdf
Copy link
Contributor

riyazdf commented Jul 20, 2016

this is awesome, LGTM pending CI!

@cyli cyli force-pushed the more-server-db-test-refactor branch 3 times, most recently from 036333b to c31aa99 Compare July 23, 2016 01:38
@endophage
Copy link
Contributor

LGTM!

@endophage
Copy link
Contributor

needs a rebase

@cyli cyli force-pushed the more-server-db-test-refactor branch from c31aa99 to 4b6eb60 Compare July 27, 2016 21:17
cyli added 4 commits July 27, 2016 15:46
…tion tests.

Convert SQL db's deletes to hard deletes, since soft deletes would prevent
us from using the same namespace in the future.

Signed-off-by: Ying Li <[email protected]>
… the rethinkdb

server health check to get info on the required tables, since listing tables does not
check the user permission.

Signed-off-by: Ying Li <[email protected]>
…dy exists in a DB, one where it doesn't

Signed-off-by: Ying Li <[email protected]>
@cyli cyli force-pushed the more-server-db-test-refactor branch from 4b6eb60 to 3dbfaa9 Compare July 27, 2016 22:46
@cyli cyli merged commit fe3a8fa into notaryproject:master Jul 27, 2016
@cyli cyli deleted the more-server-db-test-refactor branch July 27, 2016 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants