-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
added support for detecting multiple namespaces in a layer #394
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome work! mostly just nitpicks
database/models.go
Outdated
Namespaces []Namespace | ||
|
||
Features []FeatureVersion | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why you spaced these out like this? Did this pass gofmt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, the broken gofmt in emacs caused this
database/pgsql/layer.go
Outdated
return handleError("removeLayerNamespace", err) | ||
} | ||
for nsid := range namespaceIDs { | ||
_, err := tx.Exec(insertLayerNamespace, layer.ID, nsid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could all be done with one insert rather one query per namespace
database/pgsql/queries.go
Outdated
searchLayerNamespace = `SELECT n.id, n.name, n.version_format | ||
FROM Namespace n | ||
JOIN Layer_Namespace lns ON lns.namespace_id = n.id | ||
WHERE lns.layer_id = $1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation is a little funky here
7de59ab
to
ca76565
Compare
|
||
func init() { | ||
RegisterMigration(migrate.Migration{ | ||
ID: 7, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to change to 8.
database/pgsql/layer.go
Outdated
} | ||
} | ||
|
||
err = stmt.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If anything fails while iterating over the namespaceIDs, the statement never gets closed. I think it's best to structure it like this instead:
stmt, err := tx.Prepare(insertLayerNamespace)
defer func() {
err = stmt.Close()
if err != nil {
tx.Rollback()
log.WithFields(log.Fields{"error": err}).Error("failed to close prepared statement")
}()
for nsid := range namespaceIDs {
...
}
database/pgsql/queries.go
Outdated
FROM Namespace n | ||
JOIN Layer_Namespace lns ON lns.namespace_id = n.id | ||
WHERE lns.layer_id = $1 | ||
` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's move this ` to right after the $1
database/pgsql/testdata/data.sql
Outdated
(5, 'layer-3b', 1, 3); | ||
|
||
INSERT INTO layer_namespace (id, layer_id, namespace_id) VALUES | ||
(1, 2, 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent
ext/featurens/driver_test.go
Outdated
}, | ||
Files: tarutil.FilesMap{ | ||
"etc/os-release": []byte( | ||
`PRETTY_NAME="Debian GNU/Linux 8 (jessie)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put the ` on the line above and start PRETTYNAME on a newline right above NAME
database/pgsql/layer.go
Outdated
} | ||
}() | ||
|
||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this above the defer or else the defer could possibly cause a nil ptr deference
created table layer_namespace to store the many to many unique mapping of layers and namespaces changed v1 api to provide a list of namespaces for each layer changed namespace detector to use all registered detectors to detect namespaces updated tests for multiple namespaces Fixes quay#150
added support for detecting multiple namespaces in a layer
created table layer_namespace to store the many to many unique mapping of layers and namespaces
changed v1 api to provide a list of namespaces for each layer
changed namespace detector to use all registered detectors to detect namespaces
updated tests for multiple namespaces
Fixes #150