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

fix: schema_name is optional to enable future pipe grant #1424

Merged
merged 2 commits into from
Jan 10, 2023

Conversation

bennylu2
Copy link
Contributor

  • schema_name optional for future pipe grants
  • Rename futurePipes to onFuture for consistency

Test Plan

make test
CGO_ENABLED=1 go test -race -coverprofile=coverage.txt -covermode=atomic  ./...
?       github.com/Snowflake-Labs/terraform-provider-snowflake  [no test files]
ok      github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/datasources  3.711s  coverage: 3.7% of statements
?       github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/db   [no test files]
?       github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/helpers      [no test files]
ok      github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/provider     2.318s  coverage: 25.5% of statements
ok      github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/resources    14.246s coverage: 48.2% of statements
ok      github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/snowflake    1.247s  coverage: 52.0% of statements
?       github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/testhelpers  [no test files]
ok      github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/validation   0.999s  coverage: 38.0% of statements
?       github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/version      [no test files]

References

#1219

@bennylu2 bennylu2 changed the title schema_name is optional to enable future pipe grant fix: schema_name is optional to enable future pipe grant Dec 19, 2022
@sfc-gh-swinkler
Copy link
Collaborator

Hello @bennylu2 there is a merge conflict which is a bit confusing for me to decipher since the variables are getting renamed, but the error messages are the same. Can you please rebase to main branch and fix the merge conflict?

<<<<<<< future-pipe-grants
	if (schemaName == "") && !onFuture {
		return errors.New("schema_name must be set unless on_future is true.")
	}
	if (pipeName == "") && !onFuture {
		return errors.New("pipe_name must be set unless on_future is true.")
=======
	if (pipeName == "") && !futurePipes {
		return errors.New("pipe_name must be set unless on_future is true")
	}
	if (pipeName != "") && futurePipes {
		return errors.New("pipe_name must be empty if on_future is true")
>>>>>>> main

@bennylu2
Copy link
Contributor Author

@sfc-gh-swinkler rebased and ready for review

if name, ok := d.GetOk("schema_name"); ok {
schemaName = name.(string)
} else {
schemaName = ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI you don't need to set schemaName to an empty string. Since you already declared it on line 94, and the default value of a type string is an empty string.

@sfc-gh-swinkler
Copy link
Collaborator

@bennylu2 im just going to go ahead an approve this, and fix the issues with go fmt and check docs to get it out in the latest release.

For future reference, you need to run go fmt ./... and also make docs since the schema changed.

@sfc-gh-swinkler sfc-gh-swinkler merged commit 5d966fd into Snowflake-Labs:main Jan 10, 2023
@bennylu2
Copy link
Contributor Author

@bennylu2 im just going to go ahead an approve this, and fix the issues with go fmt and check docs to get it out in the latest release.

For future reference, you need to run go fmt ./... and also make docs since the schema changed.

@sfc-gh-swinkler ah thanks, will keep in mind for future. I thought I did run make docs tho

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.

2 participants