Skip to content
This repository was archived by the owner on Jul 31, 2025. It is now read-only.

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

// 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.

Although hmm... let me try: https://github.com/jinzhu/gorm/issues/342

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants