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

feat: Adding support for orgaznization and account names when creating db f… #1114

Closed

Conversation

marcin-vt
Copy link

Allowes to create database from share using organization_name.account_name notation. Add deprecation warning when using provider (based on this and this).

Supports adding comments when creating a database from share (didn't work before).

resource "snowflake_database" "db_from_share" {
  comment = "This is a comment"
  from_share = {
	  account_name = "ACCOUNT_NAME"
	  organization_name = "ORGANIZATION_NAME"
	  share = "SHARE_NAME"
	}
  name = "DB_FROM_SHARE"
}

Test Plan

  • acceptance tests
  • tested in own environment
  • acceptance tests

References

resolves #1107

@marcin-vt
Copy link
Author

Just noticed this PR #1090

@sfc-gh-swinkler sfc-gh-swinkler changed the title Adding support for orgaznization and account names when creating db f… feat: Adding support for orgaznization and account names when creating db f… Aug 4, 2022
@sfc-gh-swinkler
Copy link
Collaborator

this looks fine, but its failing the check-docs test. its unclear why, just fails with exit code 2. could you please look into this and we can get this merged?

@marcin-vt
Copy link
Author

@sfc-gh-swinkler - thanks for checking, I did correct that.
There has been some discussions around this feature here: #1107 - and in #1090. Don't know what is the approach in the end.
Also I might be unreachable for next two weeks - feel free to merge once ok with changes.

FYI @gouline.

@gouline
Copy link
Contributor

gouline commented Aug 5, 2022

@sfc-gh-swinkler Is the breaking change really warranted? At the very least provider should be kept (instead of the renamed account_name) and organization added as an optional extension (to avoid \".\"). This will cause existing databases from share to be needlessly recreated despite no actual changes (since Terraform state is different).

Also from_replica still uses \".\", so not quite sure why inconsistency in new changes is encouraged.

@marcin-vt
Copy link
Author

@gouline - prov is kept and, only 'marked' as deprecated. This should not force recreation of the db. There is a point that it is a bit inconsistent. Additional open issue not covered here is importing existing object to state and refreshing it.

@gouline
Copy link
Contributor

gouline commented Aug 5, 2022

Deprecation means it will be removed eventually, my point is that it's unnecessary. If the \".\" way of specifying the provider works for folks, it shouldn't be deprecated and organization should only be added as an optional extra for anyone who prefers it.

I'd understand the deprecation if it was a new way that improves consistency. This does the opposite.

@marcin-vt
Copy link
Author

@gouline - deprecation was just a way of pointing out that snowflake has started to call it legacy

@marcin-vt
Copy link
Author

But yeah I am fine with removing "deprecation" - as you said using prov is also working...

@gouline
Copy link
Contributor

gouline commented Aug 5, 2022

deprecation was just a way of pointing out that snowflake has started to call it legacy

Current provider is unrelated to the account identifier format, it works with either. As I responded in your issue, you can specify provider = "ORGANIZATION\".\"ACCOUNT". This is consistent with from_replica and preference expressed by @sfc-gh-swinkler in #1090 (which is why I closed it).

But yeah I am fine with removing "deprecation" - as you said using prov is also working...

Maintaining two parallel ways of defining the same feature doesn't make much sense either. I'm sorry, this PR feels like duplication of an existing feature that enforces a preference without introducing/fixing anything that cannot already be done, while also introducing an inconsistent precedent (i.e. not using fully-qualified names like in from_replica)...

@sfc-gh-swinkler
Copy link
Collaborator

I am inclined to agree with @gouline that supporting two different methods is not proper. However, I think it should be made more clear in the documentation that provider = "ORGANIZATION\".\"ACCOUNT"works. I will go ahead and close this PR

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.

Database from share using organization.account as a provider
3 participants