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: Support for selecting language in snowflake_procedure #1010

Merged
merged 10 commits into from
May 26, 2022

Conversation

emancu
Copy link
Contributor

@emancu emancu commented May 19, 2022

Implements #948

The previous behaviour was always setting JAVASCRIPT as language.
The implementation in this PR breaks the "backwards compatibility" with that rule to honour the Snowflake default: SQL.

In addition, documentation for procedure_grant and function_grant is updated too.

References

@emancu
Copy link
Contributor Author

emancu commented May 23, 2022

Do I need to do something in particular to get the approval to run the workflows?

@alldoami
Copy link
Contributor

Hi! Thank you for being patient! We were fixing some go package updates, we'll take a look at this soon! In the meantime, can you fix the conflicts?

@emancu emancu changed the title Add support for selecting language in snowflake_procedure feat: Support for selecting language in snowflake_procedure May 24, 2022
Implements Snowflake-Labs#948

The previous behaviour was always setting `JAVASCRIPT` as language.
The implementation in this PR breaks the "backwards compatibility" with that rule to honor the Snowflake default: `SQL`.
@emancu emancu force-pushed the add_language_to_procedures branch from cf5b089 to 5f89c28 Compare May 24, 2022 10:02
@emancu
Copy link
Contributor Author

emancu commented May 24, 2022

@alldoami Done, now the docs should be up to date too.

However, I'm struggling here. The definition of arguments for procedure is the same as procedure_grant and function_grant, however, the documentation generated is different!

In fact, the docs generated for procedure_grant and function_grant is incorrect (#982) but I can't figure out how to fix it 😓

@alldoami
Copy link
Contributor

Our docs are generated from the resource definition schemas! So for procedure_grant, you'll want to modify the descriptions here: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/pkg/resources/procedure_grant.go, and for function_grant, you'll want to modify the descriptions here: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/pkg/resources/function_grant.go. Does that make sense?

@alldoami
Copy link
Contributor

/ok-to-test sha=593de0a

@github-actions
Copy link

Integration tests failure for 593de0a

@emancu
Copy link
Contributor Author

emancu commented May 24, 2022

Our docs are generated from the resource definition schemas! So for procedure_grant, you'll want to modify the descriptions here: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/pkg/resources/procedure_grant.go, and for function_grant, you'll want to modify the descriptions here: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/pkg/resources/function_grant.go. Does that make sense?

Yes and no 😅 . The descriptions get updated as expected, but the problem is with the sample code generated.

Running make checks-docs generates the following example for procedure_grant and function_grant

arguments   = [
    {
      "name": "a",
      "type": "array"
    },
    {
      "name": "b",
      "type": "string"
    }
  ]

However, for procedure it generates the following (which in fact it is the correct)

  arguments {
    name = "a"
    type = "array"
  }

  arguments {
    name = "b"
    type = "string"
  }

ℹ️ The three of them share the same* definition for arguments, which it is

	"arguments": {
		Type: schema.TypeList,
		Elem: &schema.Resource{
			Schema: map[string]*schema.Schema{
				"name": {
					Type:     schema.TypeString,
					Required: true,
					Description:      "The argument name",
				},
				"type": {
					Type:     schema.TypeString,
					Required: true,
					Description:      "The argument type",
				},
			},
		},
		Optional:    true,
		Description: "List of the arguments for the procedure",
		ForceNew:    true,
	},

(*) procedure uses DiffSuppressFunc: DiffTypes but it shouldn't affect.

Am I missing something obvious? 🤔

@alldoami
Copy link
Contributor

@emancu examples are written here manually :) https://github.com/Snowflake-Labs/terraform-provider-snowflake/tree/main/examples/resources

@emancu
Copy link
Contributor Author

emancu commented May 25, 2022

@alldoami Thank you so much!
Docs are updated now 🎉

@sfc-gh-swinkler
Copy link
Collaborator

/ok-to-test sha=af431be

@github-actions
Copy link

Integration tests failure for af431be

1 similar comment
@github-actions
Copy link

Integration tests failure for af431be

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.

3 participants