Crawler: support for object ownership #2774
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This involves wiring up all the crawlers to have a workspace client, needed to locate the workspace administrator when an owner for the object cannot be found.
nfx
requested changes
Oct 1, 2024
| Iterable[User]: The active workspace administrators, if any. | ||
| """ | ||
| all_users = ws.users.list(attributes="id,active,userName,roles") | ||
| return (user for user in all_users if user.active and _has_role(user, "workspace_admin")) |
There was a problem hiding this comment.
do we have that role and it's no longer the admins group membership?
There was a problem hiding this comment.
No, this was just a guess while I figured out how it's supposed to be done. (And indeed, it now uses membership of the admins workspace group.)
| found. | ||
| """ | ||
| first_user = functools.partial(min, default=None, key=lambda user: user.name) | ||
| return first_user(find_workspace_admins(ws)) or first_user(find_account_admins(ws)) |
There was a problem hiding this comment.
don't forget to sort, because each invocation first user will be different.
…ace and account users.
…if a user directly associated with the resource cannot be located.
nfx
reviewed
Oct 9, 2024
| # Verify ownership is as expected. | ||
| ownership = ClusterOwnership(runtime_ctx.administrator_locator) | ||
| assert ownership.owner_of(cluster_record_with_owner) == ws.current_user.me().user_name | ||
| assert "@" in ownership.owner_of(cluster_record_without_owner) |
There was a problem hiding this comment.
exact user name match is preferred
Comment on lines
+47
to
+48
| def test_pipeline_ownership(ws, runtime_ctx, make_pipeline, inventory_schema, sql_backend) -> None: | ||
| """Verify the ownership can be determined for crawled pipelines.""" |
There was a problem hiding this comment.
Suggested change
| def test_pipeline_ownership(ws, runtime_ctx, make_pipeline, inventory_schema, sql_backend) -> None: | |
| """Verify the ownership can be determined for crawled pipelines.""" | |
| def test_verify_ownership_determined_for_crawled_pipelines(ws, runtime_ctx, make_pipeline, inventory_schema, sql_backend) -> None: |
|
|
||
| # Verify ownership can be made. | ||
| ownership = GrantOwnership(runtime_ctx.administrator_locator) | ||
| assert "@" in ownership.owner_of(grant_record) |
There was a problem hiding this comment.
rewrite assert "@" in ownership.owner_of(grant_record) to check for concrete usernames as much as possible, as these tests don't test enough - e.g. hiding the problem
For some reason deleting the owner of a cluster doesn't clear the creator field, even though the documentation says it should. Instead we just check that the creator is actually returned.
This was referenced Oct 9, 2024
nfx
pushed a commit
that referenced
this pull request
Oct 9, 2024
## Changes This updates an integration test to more thoroughly verify that we can locate the user-name of an administrator. ### Linked issues Follows: #2774 ### Tests - updated integration test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
As part of #2761 we need to have a way for determining the user responsible for some of our inventory types. This PR updates the crawler framework so that:
Linked issues
Progresses #2761.
Functionality
A component for locating an administrator user.
Ownership information for the following inventory types:
ClusterInfoDirectFsAccess(stubbed)GrantJobInfoPipelineInfoPolicyInfoTableTableMigrationStatusUDFTests