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

Added databricks_recipient resource for Delta Sharing #1571

Merged
merged 7 commits into from
Sep 2, 2022
Merged

Added databricks_recipient resource for Delta Sharing #1571

merged 7 commits into from
Sep 2, 2022

Conversation

stikkireddy
Copy link
Contributor

@stikkireddy stikkireddy commented Aug 30, 2022 •
edited
Loading

  • docs
  • resource
  • tests
  • supports ip access lists
  • supports db 2 db
  • supports db 2 open

Examples:

resource "databricks_recipient" "db2open" {
  name = "db2open-recipient"
  comment = "made by terraform"
  authentication_type = "TOKEN"
  sharing_code = "my code "
  ip_access_list {
    allowed_ip_addresses = [...] // fill out relevant IPv4 addresses (CIDR notation is accepted)
  }
}

resource "databricks_recipient" "db2db" {
  name = "sri-terraform-test-db2db-recipient"
  comment = "made by terraform"
  authentication_type = "DATABRICKS"
  data_recipient_global_metastore_id = "<cloud>:<region>:<guid>"
}

@stikkireddy stikkireddy requested a review from nfx August 30, 2022 14:10
@alexott
Copy link
Contributor

alexott commented Aug 30, 2022

do we always need to include

ip_access_list {
    allowed_ip_addresses = ["0.0.0.0/0"]
  }

?

nfx
nfx requested changes Aug 30, 2022
View reviewed changes
Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

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

Suggested a resource rename and left comments about example expansion. Can you also add an acceptance test? e.g. assume that metastore will be provided as TEST_DATABRICKS_METASTORE_ID environment variable in the testing environment.

docs/resources/recipient.md Outdated
name = "db2open-recipient"
comment = "made by terraform"
authentication_type = "TOKEN"
sharing_code = "my code "
Copy link
Contributor

Choose a reason for hiding this comment

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

please add tune this example with random_password resource

docs/resources/recipient.md Outdated
}

resource "databricks_recipient" "db2db" {
name = "sri-terraform-test-db2db-recipient"
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. make two separate examples (db2open & db2db) explaining what they do in one sentence.
  2. please use databricks_current_user resource instead of names.

docs/resources/recipient.md Show resolved Hide resolved
docs/resources/recipient.md Outdated
name = "sri-terraform-test-db2db-recipient"
comment = "made by terraform"
authentication_type = "DATABRICKS"
data_recipient_global_metastore_id = "<cloud>:<region>:<guid>"
Copy link
Contributor

Choose a reason for hiding this comment

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

can you expand the example with databricks_metastore resource to refer the ID from it?

docs/resources/recipient.md Outdated

The following resources are used in the same context:

* [databricks_table](../data-sources/tables.md) data to list tables within Unity Catalog.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add those in the later PRs? :)

@stikkireddy
Copy link
Contributor Author

do we always need to include

ip_access_list {
    allowed_ip_addresses = ["0.0.0.0/0"]
  }

?

AFAIK you don't @alexott it was just a poor example on my part. Ill change it to some other random public ip

nfx
nfx reviewed Aug 31, 2022
View reviewed changes
catalog/resource_recipient.go Outdated
Recipients []RecipientInfo `json:"recipients"`
}

//func (a RecipientsAPI) list() (recipients Recipients, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove commented code. we have a generated SDK for that already.

@nfx
Copy link
Contributor

nfx commented Sep 1, 2022

@stikkireddy i think it's better to remove allowed_ip_addresses line from examples and leave it as // .. fill in allowed addresses, because customers will expose the share to entire internet and would then complain to support about bad documentation.

@stikkireddy stikkireddy requested a review from nfx September 1, 2022 20:07
nfx
nfx approved these changes Sep 2, 2022
View reviewed changes
docs/resources/recipient.md Outdated
special = true
}

data databricks_current_user "current" {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
data databricks_current_user "current" {}
data "databricks_current_user" "current" {}

docs/resources/recipient.md Outdated
global metastore usually the format: `<cloud>:<region>:<guid>`

```hcl
data databricks_current_user "current" {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
data databricks_current_user "current" {}
data "databricks_current_user" "current" {}

docs/resources/recipient.md Outdated

Setting `authentication_type` type to `DATABRICKS` allows you to automatically create a provider for a recipient who
is using Databricks. To do this they would need to provide the global metastore id that you will be sharing with. The
global metastore usually the format: `<cloud>:<region>:<guid>`
Copy link
Contributor

Choose a reason for hiding this comment

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

usually the format

this reads strange

@nfx nfx enabled auto-merge (squash) September 2, 2022 19:00
@nfx nfx self-assigned this Sep 2, 2022
auto-merge was automatically disabled September 2, 2022 19:04

Head branch was pushed to by a user without write access

@stikkireddy
Merge branch 'master' of github.com:stikkireddy/terraform-provider-da…
8c48af1 
…tabricks into delta-sharing/recipient-resource
@nfx nfx changed the title Delta sharing recipient resource Added databricks_recipient resource for Delta Sharing Sep 2, 2022
@nfx nfx merged commit 12210ff into databricks:master Sep 2, 2022
@nfx nfx mentioned this pull request Sep 12, 2022
Merged
michael-berk pushed a commit to michael-berk/terraform-provider-databricks that referenced this pull request Feb 15, 2023
@stikkireddy
Added databricks_recipient resource for Delta Sharing (databricks#1571
c315d74 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wchau wchau wchau left review comments

@nfx nfx nfx approved these changes

Assignees

@nfx nfx

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stikkireddy @alexott @nfx @wchau