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

Ownership Attribute #208

Merged
merged 10 commits into from
Jun 22, 2023
Merged

Ownership Attribute #208

merged 10 commits into from
Jun 22, 2023

Conversation

dehume
Copy link
Contributor

@dehume dehume commented Jun 19, 2023

Removing ownership as it's own resource type and instead adding it as an attribute on existing resources (using tables as an example).

This adds two new columns to the resource ownership_role and ownership_id. The ownership role is an optional field that the user can set. If set during creation, an ALTER... OWNER will be run immediately after the CREATE. Then as part of the read, the ownership id will be read in as a computed field. Even if the ownership role is not set, the ownership id is set as all objects have a default owner.

This will behave very similarly on updates. If the user changes the ownership role, an ALTER... OWNER is run and the new owner id is updated on the read. However since there is no revoking of ownership if the user removes the ownership role field, no alter is run and the state of the ownership role is set to nothing, though the owner id will still reflect the ownership maintained by Materialize.

Since we are now running a CREATE and ALTER as part of the create, there should probably be more sophisticated error handling. If you tried to say create a table with an ownership role using a role that does not exist, the create would be successful but the alter would not and how would we want to handle that failure.

NOTE: Changing the builder factories to take in an object rather than the string parameters of name, schema, database so that object can be more easily shared across the multiple builders used in the create resource.

@dehume dehume requested review from benesch and bobbyiliev June 19, 2023 15:22

}

func TestResourceTableUpdate(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the update tests since this is just much easier and more accurate with the acceptance tests.

@dehume dehume linked an issue Jun 19, 2023 that may be closed by this pull request
@benesch
Copy link
Member

benesch commented Jun 20, 2023

Very nice!

Then as part of the read, the ownership id will be read in as a computed field. Even if the ownership role is not set, the ownership id is set as all objects have a default owner.

This surprised me. The Terraform provider deals in names rather than identifiers approximately everywhere. Does something go wrong if we just join the owner_id against the roles table to pull out the role name? Then ownership_role is whatever you've set it to, unless you haven't set it to anything, in which case it's whatever role Materialize has automatically assigned.

@dehume
Copy link
Contributor Author

dehume commented Jun 20, 2023

Does something go wrong if we just join the owner_id against the roles table to pull out the role name?

I think my hangup was not wanting to change the base queries but adding another join is pretty easy. Removed the ownership_id field so there is just done with the role name.

@dehume
Copy link
Contributor Author

dehume commented Jun 21, 2023

Incorporated the changes necessary from #209 which was a big PR and also added ownership for clusters to see if the patterned would hold for another resource. I think this is good to merge and will move adding ownership for the other resources to a separate PR.

@dehume dehume merged commit c3c61f6 into main Jun 22, 2023
@dehume dehume deleted the Ownership-Attribute branch June 22, 2023 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resource property ownership
3 participants