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.

Sign up for GitHub

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

Jump to bottom

feat: initial implementation of metabase group migration into guardian #167

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from
Draft

feat: initial implementation of metabase group migration into guardian #167

wants to merge 21 commits into from

Conversation

singhvikash11
Copy link
Member

No description provided.

@singhvikash11
feat: adding table, group as metabase resource and add groups-databas…
47c3881 
…es to resources
@singhvikash11
fix: fix test case
61580ff
@singhvikash11
Fix test case
bf17893 
Add permission to group
Add logs
@singhvikash11
Add constants for string literal
16f517b
@singhvikash11
Change type for database and collection in group
e79c96e
@singhvikash11
Resolve feedback
2d93d9c
@singhvikash11
fix:
4257dd2 
creation of existing group
users already added to a group
grant access for group
revoke access for group
@singhvikash11
Add grant and revoke method for group, table
1c1f807
@singhvikash11
Fix test
f8e6386
@singhvikash11
Fix lint
eda6c8c
@singhvikash11
Resolve feedback and add fetch database/collection while fetching groups
76c6510
@singhvikash11
fix: custom group naming convention for metabase provider and filter …
5d01b7c 
…groups created by guardian
@singhvikash11
fix: custom group naming convention for metabase provider and filter …
1268389 
…groups created by guardian
@singhvikash11
Merge remote-tracking branch 'origin/metabase_resources' into metabas…
15ba0eb 
…e_resources
@singhvikash11
fix: resolve merge conflict
207ac02
@singhvikash11
fix: update the log to use key/value pair logging
38ffeed
@singhvikash11
feat: initial implementation of metabase group migration into guardian
158ea99
@singhvikash11 singhvikash11 marked this pull request as draft May 19, 2022 11:45
ravisuhag
ravisuhag reviewed May 23, 2022
View reviewed changes
cmd/migration.go
func MigrationCmd(config *Config) *cobra.Command {
cmd := &cobra.Command{
Use: "migrate",
Short: "Guardian migration",
Copy link
Member

Choose a reason for hiding this comment

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

@rahmatrhd @singhvikash11 I think we need to think of a better approach than attaching it on migrate. One possible suggestion could be to call it import and scope it with the provider itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ravisuhag this is a good idea, we need to implement migrate for bigquery provider and other providers too. Then we could expose it as a sub-command on the provider

Copy link
Member Author

Choose a reason for hiding this comment

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

@ravisuhag @rahmatrhd If you folks are okay with Metabase migrate for now then I can move this as sub-command to provider.

Copy link
Member

@ravisuhag ravisuhag May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@singhvikash11 What do you mean okay with metabase provider?

Also, i would recommend checking out https://github.com/GoogleCloudPlatform/terraformer for some inspiration on how command could look like.

guardian provider import <provide-name> -r <resources>

what do you guys think? cc @rahmatrhd

Copy link
Member Author

@singhvikash11 singhvikash11 Jun 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@ravisuhag we already have the command guardian provider create command to register the resources with guardian. Let's use the following command as suggested by you to import ACL of existing resources from a source system:

guardian provider import <provide-name> acl
guardian provider import <provide-name> acl -r <resources>

what do you guys think? cc @rahmatrhd

Copy link
Member

Choose a reason for hiding this comment

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

@singhvikash11 What does acl represent in this command?

Copy link
Member Author

@singhvikash11 singhvikash11 Jun 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@ravisuhag Bigquery, Metabase resources like table, dataset, collection etc are defined by a set of permissions associated with a set of people, groups, service-account in the source system. By importing access-control of these resources into our system we'll replicate it in the model of appeal-resource in Guardian.

Copy link
Member

Choose a reason for hiding this comment

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

@singhvikash11 Yes, understood. But what i meant is that we don't need acl word in the comamnd right? the command can plainly be guardian provider import <provide-name> -r <resources> where -r flag will take the kind of resources you want to import from that provider.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ravisuhag This makes sense, we can go ahead with this command.

cc @rahmatrhd

@ravisuhag ravisuhag mentioned this pull request Jul 7, 2022
Closed
@ravisuhag ravisuhag linked an issue Jul 8, 2022 that may be closed by this pull request
Import pre-existing access from providers #206
Closed
@ravisuhag
Copy link
Member

ravisuhag commented Nov 12, 2022 •
edited
Loading

@singhvikash11 @rahmatrhd Given provider import is done. Can we move this to use that contract and update the PR?

@ravisuhag
Copy link
Member

@rahmatrhd @bsushmith Is there any plan to pick this up?

@ravisuhag ravisuhag force-pushed the main branch 4 times, most recently from fdb418b to cb600c1 Compare June 30, 2023 01:39
lifosmin referenced this pull request in lifosmin/guardian Aug 31, 2023
@rahmatrhd
feat(guardian): add APIs to add and remove approver (goto#167)
60db513
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ravisuhag ravisuhag ravisuhag left review comments

@rahmatrhd rahmatrhd Awaiting requested review from rahmatrhd rahmatrhd is a code owner

@AkarshSatija AkarshSatija Awaiting requested review from AkarshSatija AkarshSatija will be requested when the pull request is marked ready for review AkarshSatija is a code owner

@mabdh mabdh Awaiting requested review from mabdh mabdh will be requested when the pull request is marked ready for review mabdh is a code owner

@bsushmith bsushmith Awaiting requested review from bsushmith bsushmith will be requested when the pull request is marked ready for review bsushmith is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
provider
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Import pre-existing access from providers
3 participants
@singhvikash11 @ravisuhag @rahmatrhd