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

group classification is too inclusive for model access #86

Closed
dave-connors-3 opened this issue Jun 29, 2023 · 3 comments · Fixed by #89
Closed

group classification is too inclusive for model access #86

dave-connors-3 opened this issue Jun 29, 2023 · 3 comments · Fixed by #89
Assignees

Comments

@dave-connors-3
Copy link
Collaborator

Grace did the following:

I just did the following workflow:

  1. Ok, I know I want to create a new group for all of my models in my 1__stage, 2__raw_vault, and 3__business_vault folders. I execute: dbt-meshify group vault --owner-name "Trainer Logan" -s "1__stage 2__raw_vault 3__business_vault"
  2. Because I've only built out a few models that build on those "vault" models, some are marked as public and some are marked as private. But actually, I want all of my models in 2__raw_vault and 3__business_vault folders to be public so that anyone can access and use them. So I executed dbt-meshify operation add-contract -s "2__raw_vault 3__business_vault"
  3. But that just added contracts, it didn't update my access. So I had to go in and manually change the access config for all of the models in those folders.

I'm okay with not automatically updating access when you add a contract, but I do think we then need an operation to update access manually.

Originally posted by @graciegoheen in #85 (comment)

The reason number 2 did not properly classify her resource types appropriately based on her selection syntax is that we're doing the classify_resource_access step on all resources types in the selected group. Instead, we should limit the interface analysis to model nodes only, as they are the ones that should have access settings at all!

once the split command PR is merged, we'll have yml operations for all resource types, so we can add the group config to all relevant resource types, and limit the access type step to just models

@dave-connors-3
Copy link
Collaborator Author

@nicholasyager I think the solve for models would be a one liner here to filter nodes to model nodes only before we call cls.classify_resource_access

@nicholasyager
Copy link
Collaborator

The reason number 2 did not properly classify her resource types appropriately based on her selection syntax is that we're doing the classify_resource_access step on all resources types in the selected group. Instead, we should limit the interface analysis to model nodes only, as they are the ones that should have access settings at all!

I agree strongly that we can and should update _generate_resource_group to only have models be processed by classify_resource_access.

Having that been said, I do not believe that #2 is caused by this. Rather, having some models in 2__raw_vault as public and some as private is an artifact of how the access inference code works. Fundamentally, if the entire group (or subsets of the group) are being forced into a specific access level, this would (in my opinion) suggest the need for a specific access level assignment command (and a rethinking of what these groups are actually accomplishing!)

@dave-connors-3
Copy link
Collaborator Author

in grace's case, test nodes that were one level downstream of leaf model nodes were being assigned public, while the leaf model nodes were being marked private, which caused the initial confusion! had we only considered the models as boundary nodes, I think she would have seen the behavior she wanted, and wouldn't have needed to to anything else to try to make those access settings different.

i do think we're aligned that a access-specific operation command would help either way!

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