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

Make description unique for tenant groups in MiqGroup #19272

Merged

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Sep 9, 2019

After #17197 we distinguish case sensitive for column description in groups and there is no such validation for Tenant#name.

When we create tenant then we are also creating so called tenant group and this group has generated description - base on related tenant name.

Issue:
A. create tenant with name = foo and description =foo
=> tenant (with name=foo and description =foo) is created
=> also related tenant group has been created with description (Tenant foo access )
B. create tenant with name = FOO and description =foo
=> tenant is not created because when default group is created it will generate description which was created in point 1 and it will fails on validation.

What we can do ?

  1. Make description for tenant group unique this PR.
  2. Revert Add case insensitivity when validating uniqueness of name of new group/role #17197
  3. Add similar validation for Tenant#name. (but in this case what to do with already created tenants with duplicated names "FOO", "Foo" ?)

Links

cc @kbrock @Hyperkid123

WIP: Need to know whether this is good approach.

@miq-bot miq-bot added the wip label Sep 9, 2019
@Hyperkid123
Copy link

Just wanted to add that this is blocking: ManageIQ/manageiq-ui-classic#6143

@miq-bot
Copy link
Member

miq-bot commented Sep 9, 2019

Checked commit lpichler@27618dc with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@Hyperkid123
Copy link

Also this now prevents users to create tenants with Foo/foo names. But the API does return 200 response even if though the tenant is not created. We should resolve this ASAP. At least make the API create_resource single transaction.

@kbrock
Copy link
Member

kbrock commented Sep 9, 2019

I thought there was uniqueness among parents.

So you could have the same tenant name, as long as it had a different parent.

Not sure if this still works - and if it is a good approach. just saying the original (un-informed) decision

@Hyperkid123
Copy link

Hyperkid123 commented Sep 9, 2019

@kbrock the problem is miq_group not the Tenant itself. And if the tenants have same parent it will generate the same group description which is case insensitive and that will cause the problem

Basically if there is a tenant with parent "My Company" and name "Foo" it will generate group description "Group My Compnay/Foo"

If you then want to create another tenant called just foo it will generate "Group My Company/foo" and is being case insensitively validate "Group My Compnay/Foo" === "Group My Company/foo" which will fail and the group is not created which will subsequently break the tenant.

@kbrock
Copy link
Member

kbrock commented Sep 9, 2019

@Hyperkid123 well, case sensitivity is a problem. it should be case insensitive for sure

@Hyperkid123
Copy link

@kbrock so should we add to tenants as well then? I have no problem with that i just want a solution so we can move forward.

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Sorry - I was not reading properly

@lpichler Think we're ready to merge (ping me when you are ready)

@lpichler lpichler changed the title [WIP] Make description unique for tenant groups in MiqGroup Make description unique for tenant groups in MiqGroup Sep 11, 2019
@miq-bot miq-bot removed the wip label Sep 11, 2019
@kbrock kbrock merged commit 6a387a9 into ManageIQ:master Sep 11, 2019
@kbrock kbrock self-assigned this Sep 11, 2019
@kbrock kbrock added this to the Sprint 120 Ending Sep 16, 2019 milestone Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants