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

featuturefmt: Extract PotentialNamespace #722

Merged
merged 6 commits into from
Mar 8, 2019
Merged

Conversation

Allda
Copy link
Collaborator

@Allda Allda commented Feb 28, 2019

PotentialNamespace is feature namespace extracted while detecting features in a layer. The PotentialNamespace is stored in nemaspace table + reference is also stored in layer_feature.

This will allow having multiple namespaces for single layer feature.

Partially resolves: #706

database/models.go Outdated Show resolved Hide resolved
@Allda
Copy link
Collaborator Author

Allda commented Mar 6, 2019

@KeyboardNerd The PR is ready to be reviewed.

Here is the feature lister which uses a potential namespace which I used for testing.
https://gist.github.com/Allda/99d30f5d37d35dd3e2d0244470d6f150

There is one thing which is causing problems now and I believe it will be resolved once you made your changes in updater.go

When feature lister detects multiple namespaces (cpes) it returns each package multiple times with different CPE. It works fine, store namespaces, features, layer_feature, but it fails when storing data to ancestry_feature because of duplicated reference to namespaced_feature_id.

@Allda
Copy link
Collaborator Author

Allda commented Mar 6, 2019

I found one more bug, wait with a review, please.

@Allda
Copy link
Collaborator Author

Allda commented Mar 6, 2019

I found one more bug, wait with a review, please.

Fixed

WHERE lf.feature_id = f.id
AND t.id = f.type
AND lf.namespace_id = ns.id
Copy link
Contributor

Choose a reason for hiding this comment

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

what if ls.namespace_id is null? there won't be a matched ns.id.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Resolved by using Left join

database/dbutil.go Outdated Show resolved Hide resolved
@KeyboardNerd
Copy link
Contributor

Schema looks ok.

Please add some tests on the change, especially the features with potential namespace.

PotentialNamespace is feature namespace extracted while detecting
features in layer. It will server for special feature detector. The
current detectors return empty namespace.
PotentialNamespace is part of layer_feature table and it is also stored
in namespace table.
PotentialNamespace should be in LayerFeature instead of Feature struct.
Feature extractors were updated to return LayerFeature instead of
Feature.
If layer contains more than one potential namespace, features will be
created for each namespace. Layer_feature table now has to contains one
more constrains (namespace_id).
@Allda Allda force-pushed the feature_ns branch 2 times, most recently from 8a9db6e to 16c9f37 Compare March 7, 2019 13:03
@@ -2,7 +2,8 @@
INSERT INTO namespace (id, name, version_format) VALUES
(1, 'debian:7', 'dpkg'),
(2, 'debian:8', 'dpkg'),
(3, 'fake:1.0', 'rpm');
(3, 'fake:1.0', 'rpm'),
(4, 'cpe:/o:redhat:enterprise_linux:7::server', 'rpm');
Copy link
Contributor

Choose a reason for hiding this comment

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

Does 'cpe:/o:redhat:enterprise_linux:7::server' has any version information?

If this is the actual format for CPE, I'll have to change the namespace db a bit because right now we assume namespace name consists of two components: <name>:<version> as a string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please rebase to master, and test. We found a bug in our tests that prevents database test failure from showing up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The version is usually included in CPE (in this case it is 7), but from the definition of CPE the version can be omitted.

https://access.redhat.com/blogs/766093/posts/2387501

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I noticed the bug. Today I spend a few hours trying to figure out why my test is not failing and after I found the cause and I fixed it I noticed that you fixed it yesterday

Copy link
Contributor

@KeyboardNerd KeyboardNerd left a comment

Choose a reason for hiding this comment

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

A few points, I'll do extra testing today.

database/pgsql/feature.go Outdated Show resolved Hide resolved
database/pgsql/layer_test.go Outdated Show resolved Hide resolved
Test verifies that potential namespace is stored in database and it can
be loaded back to structure.

The commit also fixes few typos and bugs.
String was used when Feature contains PotentialNamespace. Since it was
moved to LayerFeature we can use struct as map key instead of string.
Copy link
Contributor

@KeyboardNerd KeyboardNerd left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Enable multiple namespaces for installed layer feature
2 participants