Add tenant_id to miq_product_features

[lpichler]

issue ManageIQ/manageiq#18100

tenant_id establish relation between dynamic product features and tenants

from MiqUserRole:

cc @gtanzillo

Links

https://bugzilla.redhat.com/show_bug.cgi?id=1297415

[carbonin]

What is the tenant_node column used for here? I tried to read through the BZ and issue, but I don't see that called out anywhere.

[lpichler]

@carbonin yes I didn't comment it yet - I updated description with explanation. (I'll update other main issue with it)

[carbonin]

Hmm I think we can come up with a better name than tenant_node. My first thought was that since the parent_id column forms a tree of features, every one would be a "node" in that tree. So maybe tenant_node meant that these are the only product features that belong to a tenant, but it looks like more of the opposite, right?

If you have tenant_node = true then it means "this feature is the root for the tenant-scoped features that we might have to create" and, in fact, this particular feature probably won't belong to a specific tenant, right?

Would something like tenant_scoped_feature_root make more sense?

[carbonin]

Also would like @gtanzillo to 👍 these changes, as he probably has a better idea of the larger picture we're trying to solve.

[JPrause]

@miq-bot add_label blocker

[carbonin]

Why was this marked a blocker @JPrause? The linked BZ does not have the blocker flag and the Target Release is set to (what would be) Ivanchuk upstream.

[gtanzillo]

Regarding @carbonin suggestion for the column name

Would something like tenant_scoped_feature_root make more sense?

How about just tenant_scoped? Because that's the exact purpose of this boolean. It indicated that this product feature node should be scoped for each tenant.

[carbonin]

@gtanzillo I think that would be based on what the value of the column would be for the features in @lpichler's picture.

Is just the "Tenant Node Product Feature" labeled as tenant_node = true or are all of the "(Dynamic) Tenant Product Features" tenant_node = true as well?

On a separate note @gtanzillo your name suggestion makes me think this might not need to be a column at all if it applies to any row which relates to a tenant. Could we make a scope for where.not(tenant_id => nil) and have a tenant_scoped method that just checks the tenant id in that case?

[lpichler]

"Tenant Node Product Feature" is tenant_node = true and tenant_id is nil (it is not related to any certain tenant)
"(Dynamic) Tenant Product Features" is tenant_node = false and tenant_id is not nil
"Existing Product Features" is tenant_node = false (or nil) and tenant_id is nil

and I think it is not enough to have just where.not(tenant_id => nil) to distinguish these 3 situations. Or Am I wrong ?

@carbonin

1. so are you to keep this 2 columns ?
2. are you ok with tenant_scoped name of the column ?

[carbonin]

@lpichler based on your description I think tenant_scoped is not a good name.

In my mind anything that is assigned to a tenant would be "tenant scoped" so that's not really what this column means.

I would rather avoid giving meaning to nil either way so it sounds like we still need two columns.

I think the best description is still "this feature is the root of the tree for features which apply to a particular tenant" and I would like that to come across in the name. Maybe just tenant_features_root?

[lpichler]

thanks @carbonin

yes it is good idea to have "parent" or "root" in name the in order to describe it better: tenant_features_root I am good with this 👍

@gtanzillo do you agree ?

[carbonin]

I also talked a bit with @gtanzillo about if the second column is even needed. Will you ever need to query for nodes which should be the root of tenant specific features? Isn't that more of a known set that we're going to implement?

I think I'm clear on what you're referring to, but not clear on why it needs to be a column.

[lpichler]

@carbonin

we don't need query for any other reasons we need it only in MiqProductFeature#seed method:

The only thing why we need MiqProductFeature#tenant_features_root column is: mark 'somehow' concrete (existing) product feature in https://github.com/ManageIQ/manageiq/blob/master/db/fixtures/miq_product_features.yml#L2457
in order to become the product feature as "Tenant Node Product Feature".

Thanks to that we can dynamically generate "Dynamic Tenant Product Feature" under the "Tenant Node Product Feature".

We had solution that I added new type for feature_type (to be distinguish "Tenant Node Product Feature". ) but we did not considered this solution as safe and beneficial (we don't want to break UI )
as well as adding a new column MiqProductFeature#tenant_features_root.

so If you have any other idea how to determine "Tenant Node Product Feature". let me know.

Maybe we can have other yaml which will be list "Tenant Node Product Features". ?

thanks!

[carbonin]

@lpichler is MiqProductFeature#identifier unique? We could have a constant in MiqProductFeature that lists the feature identifiers for which we should dynamically create tenant product features.

Then MiqProductFeature#tenant_features_root? would be implemented as TENANT_FEATURE_ROOT_IDENTIFIERS.include?(identifier)

Does that make sense?

[carbonin]

I guess from a more general standpoint I'm saying that something should only be added to the database (rather than solved in a model) if it changes at runtime or needs to be referenced in a query (via joins or as a query condition).

It sounds like neither of these is true in this case.

[miq-bot]

Checked commit lpichler@f77df0b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

[lpichler]

@carbonin yes is MiqProductFeature#identifier is unique. it does make sense to have TENANT_FEATURE_ROOT_IDENTIFIERS 👍

So I am leaving here only tenant_id and I updated description.

so now it could be merged ?

and thanks 👍

[lpichler]

@miq-bot add_label hammer/yes

[simaishi]

Hammer backport details:

$ git log -1
commit 3531941f063dc6e71b26d4190fb2a94eddb72f58
Author: Nick Carboni <[email protected]>
Date:   Fri Oct 26 11:45:24 2018 -0400

    Merge pull request #291 from lpichler/add_tenant_id_to_miq_product_features
    
    Add tenant_id to miq_product_features
    
    (cherry picked from commit ed6309af48540b9adb32ed9c999a9b1acdde3458)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1468795