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

support multiple namespaces in one layer #157

Closed
wants to merge 6 commits into from
Closed

support multiple namespaces in one layer #157

wants to merge 6 commits into from

Conversation

liangchenye
Copy link
Contributor

Signed-off-by: liang chenye liangchenye@huawei.com

Signed-off-by: liang chenye <liangchenye@huawei.com>
@liangchenye
Copy link
Contributor Author

liangchenye commented Apr 25, 2016

Hi @jzelinskie @Quentin-M , the PR follows the suggestion from you in #150, PTAL.

It makes the following changes:

  1. change *namespace to []namespace in struct Layer
  2. return 0~N namespaces while detecting a layer and let the underling feature detector to decide which namespace does it support
  3. add a new table - LayerNamespace
    remove namespace_id from talbe Layer and add the related soi/search/delete SQL accordingly.
    When insert a layer into the database, merge its own namespace with its parent's namespace
    and insert them into LayerNamespace.
  4. modify the related test cases

@jzelinskie jzelinskie added kind/design relates to the fundamental design of a component lacking/review needs to be reviewed by a maintainer component/database labels Apr 25, 2016
if layer.Namespace != nil {
n, err := pgSQL.insertNamespace(*layer.Namespace)
// Find its parent's namespaces
rows, err := pgSQL.Query(searchLayerNamespace, parentID)
Copy link
Contributor

@Quentin-M Quentin-M Apr 25, 2016

Choose a reason for hiding this comment

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

Why aren't layer.Parent.Namespaces already loaded? We already make the assumption that the provided Parent layer has been retrieved from the database (see L255).

Copy link
Contributor Author

@liangchenye liangchenye Apr 26, 2016

Choose a reason for hiding this comment

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

The layer.Parent.Namespaces are loaded in work.go.
But in layer_test, it is not. In FindLayer (around L55), the parent's ID and name are loaded, but parent's namespace is not.

As we already make the assumption that the provided Parent layer has been retrieved.
So either we modify the related test cases or load parent's namespace in FindLayer.
My second commits did the later choice, it is easier for developers who use database interface directly but it will be time consuming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in new PR, I'll update test cases. So will not reload parent's namespaces.

Signed-off-by: liang chenye <liangchenye@huawei.com>
@@ -35,7 +35,7 @@ type Error struct {

type Layer struct {
Name string `json:"Name,omitempty"`
NamespaceName string `json:"NamespaceName,omitempty"`
NamespacesName []string `json:"NamespacesName,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

NamespaceNames

@jzelinskie
Copy link
Contributor

When complete, this change will also need a database migration to move all current installations to the new schema.

Signed-off-by: liang chenye <liangchenye@huawei.com>
Signed-off-by: liang chenye <liangchenye@huawei.com>
Signed-off-by: liang chenye <liangchenye@huawei.com>
@liangchenye
Copy link
Contributor Author

Add 'version' field to Namespace. (commit: add 'version' to namespace')

I have two questions:

  1. Since it breaks V1, should there be a V2 branch?
    (commit: testing commit: modify V1 by add Namespace.Version)
  2. about the database migration, is that mean should we keep the origial 2015*.sql and anther sql file? Also, should we add a .go file to migrate v1 data to this new version?

Signed-off-by: liang chenye <liangchenye@huawei.com>
@jzelinskie jzelinskie added reviewed/needs rework will be closed if review not addressed and removed lacking/review needs to be reviewed by a maintainer labels May 16, 2016
@jzelinskie
Copy link
Contributor

jzelinskie commented May 16, 2016

This should be rebased now that it cannot be merged anymore and then reviewed again.

Master is going to be the branch for "v2" work, we're managing the branches similar to etcd where we have branches for each minor release.

The idea behind migrations is to always move forward by adding new files. This includes writing code (not just SQL) to upgrade from the old data schemas.

@liangchenye
Copy link
Contributor Author

liangchenye commented May 24, 2016

Rebased in my branch https://github.com/liangchenye/clair/tree/multiple-detector.

I don't know why it becomes an 'unknown' repository. Once the upgrading code was done, I'll add a new PR and close this one.

@liangchenye
Copy link
Contributor Author

Thanks @jzelinskie , some create/alter table work could be done by SQL, but the sequence between SQL and code matters. So now I'm working on adding a pure migration code.

@liangchenye
Copy link
Contributor Author

liangchenye commented May 25, 2016

Hi @jzelinskie , I don't know if you have ever meet the same problem in goose.
Seems goose does not work well with 'vendor' ?

And another thing is, a user has to have goose lib under his/her GOPATH directory.
The goose golang migration is quite different with goose sql migration, it genereates .go files under a /tmp/goose* directory and then 'go run' them. So Clair will depend on a go-dev environment , is it acceptable?

@liangchenye
Copy link
Contributor Author

As comment above, .go migration is not well supported by goose. (to my knowledge).
I change to use 'pure' sql way.
It works good and pass tests :) https://github.com/liangchenye/clair/tree/multiple-detector.
I'll review that thoroughly and submit again next week.

@liangchenye
Copy link
Contributor Author

close and work on #193

@Quentin-M
Copy link
Contributor

Hi,

Thank you again for your work on this.

liamstask/goose is clearly problematic. Issue #93 tracks it. There is a terrible lack of solid solution for database migration in Go. As far as I know, none of them except liamstask/goose support doing migrations using go code and none can do it without having a go env. That is a huge drawback for any Go app that use databases and that can pretend to be ran in production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/design relates to the fundamental design of a component reviewed/needs rework will be closed if review not addressed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants