-
Notifications
You must be signed in to change notification settings - Fork 427
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: Add support for cortex search service #2860
feat: Add support for cortex search service #2860
Conversation
@chris-codaio Thank you so much for putting this together! I'm going to take a look at the compilation error you hit and then circle back to this PR. @sfc-gh-asawicki would you be able to help find the right reviewer for this? |
Hey @chris-codaio @sfc-gh-mloring. Thanks for the contribution! We will review it in a couple of days (as described in https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CONTRIBUTING.md#requesting-the-review). Before that please make sure that the PR follows all the requirements described in https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CONTRIBUTING.md#contributing. (one thing I noticed from the first glance is that the SDK part is created manually and not using the existing generator: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CONTRIBUTING.md#introducing-a-new-part-of-the-sdk). |
Hello @sfc-gh-asawicki , I initially skipped the generator given the lack of documentation linking how to setup various field structures from documentation to its internal builder. I just slogged my way through it - hope that looks reasonable? |
Hi @chris-codaio, we will review this PR once this is available in our testing environment, can't promise any specific date. |
Your test accounts should be enabled now. |
Hey @chris-codaio @sfc-gh-mloring, I just wanted to let you know that we had to postpone the tests and review because of internal issues that the team had to tend to. Currently, I estimate we will be able to spend time on it on Friday. We are sorry for the delay. The closet release is expected at the end of next week or later, so it is still possible that this PR will be a part of it. |
Hey @chris-codaio @sfc-gh-mloring. Cross-posting the questions asked internally for transparency. We would like to have the answers before proceeding with this PR because the answers will affect the design of the resource.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @chris-codaio, Thanks again for the contribution. I've added some comments, but some of them are affected by the answer to the @sfc-gh-asawicki question/list of things we have to know. You can skip those for now, but the rest should be valid comments.
2b97a37
to
b27b382
Compare
b27b382
to
456eb62
Compare
Thank you for taking such a detailed look and for suggesting so many improvements to our docs. I'll work with our doc writer to get these updated!
Attributes are optional, we'll update the docs.
Looks like a word is missing.
These are included in describe output in a recent change that is still waiting to roll out to production.
Looks like a word is missing here too.
Yep, these are optional. We'll get them removed from the docs.
We don't have near term plans for these
Yes. As we move from private preview to public preview, we expect to include more information in both show and describe.
The change to add this to describe has already landed. The changes to show are in progress.
Attributes are the set of columns that you want to perform filtering on at search time. Any columns specified in the projection list of the source query that are not named as "attributes" are included columns. These columns cannot be searched over or filtered on but can be included in search results if desired.
We're still in private preview and both show and describe output is in flux. We'll get the docs updated once we've decided on the final structure. Is terraform dependent on the specific output format of show/describe or are these questions more general?
Thanks for catching. We'll update the docs. |
Ech, sorry GH markdown thought that
Okay, this affects the potential implementation in the provider.
Same as above:
They are really helpful for a proper terraform provider implementation. Without them we have to drop and recreate the resource with almost every change.
From the provider point of view: every parameter used in create should be returned in SHOW or DESCRIBE; this is how we can discover the changes both in config and the external ones (e.g. manual in worksheet) from the provider side.
Terraform is highly dependent on the information that can be retrieved from SHOW/DESC (as in one of the points above). |
@sfc-gh-asawicki @sfc-gh-mloring - I've made a bunch of updates to address the outstanding feedback. A couple of remaining questions for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked the change, and I like it. Thanks for all the changes after @sfc-gh-jcieslak reviews. I have left a few small comments/answers but I would like to proceed and merge this change.
We have some stuff we have to add before it gets released so we can also cover a few more changes/corrections. Just FYI for you @chris-codaio, for future reference (we still update our contribution guidelines with the conventions that our community is not aware of and I will add the following topics):
- import instructions (they have to be added as a separate file in the examples directory so they can be automatically added to the generated docs)
- examples (same as above)
- test for added privileges (because the object is new, we have to test its correctness with at least an integration test on the SDK level before we can release it)
- in the SDK tests we tend to keep them all inside one test wrapping function (not a function dedicated for each group of requests - this helps with shorter set ups)
- formatting and docs regeneration (I noticed that make pre-push was not run after the latest changes)
- [this one is a more of a nit-pick] - instead of checking the created date for the changes on the objects, we can check the plan for the resource (planchecks.ExpectResourceAction)
- (the ones from my comments): missing forceNew, grants to be removed
https://pkg.go.dev/time | ||
note: format may depend on what the account parameter for TIMESTAMP_OUTPUT_FORMAT is set to. Perhaps we should return this as a string rather than a time.Time? | ||
*/ | ||
record["created_on"] = dt.CreatedOn.Format("2006-01-02T16:04:05.000 -0700") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's do String() here pls.
Columns: []string{"product_id", "product_name"}, | ||
} | ||
opts.Comment = String("comment") | ||
assertOptsValidAndSQLEquals(t, opts, `CREATE CORTEX SEARCH SERVICE IF NOT EXISTS %s ON searchable_text ATTRIBUTES product_id, product_name WAREHOUSE = "warehouse_name" TARGET_LAG = '1 minutes' COMMENT = 'comment' AS SELECT product_id, product_name, searchable_text FROM staging_table`, id.FullyQualifiedName()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the answer we get from @sfc-gh-mloring I would say it's fine:
Yep, these are optional. We'll get them removed from the docs.
@@ -204,6 +206,7 @@ const ( | |||
PluralObjectTypeShares PluralObjectType = "SHARES" | |||
PluralObjectTypeTables PluralObjectType = "TABLES" | |||
PluralObjectTypeDynamicTables PluralObjectType = "DYNAMIC TABLES" | |||
PluralObjectTypeCortexSearchServices PluralObjectType = "CORTEX SEARCH SERVICES" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, let's remvoe them.
Also, we should have a test if granting the privilege works for the singular object
/ok-to-test sha=4c0f856994da281a20b3f2fb29e6a02477d9291e |
Integration tests failure for 4c0f856994da281a20b3f2fb29e6a02477d9291e |
1 similar comment
Integration tests failure for 4c0f856994da281a20b3f2fb29e6a02477d9291e |
🤖 I have created a release *beep* *boop* --- ## [0.93.0](v0.92.0...v0.93.0) (2024-07-10) ### 🎉 **What's new:** * Add OAUTH integration for custom clients ([#2908](#2908)) ([d9b557f](d9b557f)) * Add oauth integration for partner applications ([#2912](#2912)) ([91788e5](91788e5)) * Add support for cortex search service ([#2860](#2860)) ([43aa89f](43aa89f)) * API Authentication integration v1 readiness ([#2898](#2898)) ([91931da](91931da)) * External Oauth integration v1 readiness ([#2907](#2907)) ([ed237c3](ed237c3)) * Generate show outputs with mappers ([#2886](#2886)) ([1cada88](1cada88)) * Introduce security integrations datasource ([#2892](#2892)) ([7f6c657](7f6c657)) * SAML2 integration v1 readiness ([#2868](#2868)) ([d0c136d](d0c136d)) * SCIM integration v1 readiness ([#2846](#2846)) ([269df6b](269df6b)) * Security integrations datasource v1 readiness ([#2913](#2913)) ([d10474a](d10474a)) * standard database v1 readiness ([#2842](#2842)) ([3c11953](3c11953)) * Warehouse redesign final touches ([#2900](#2900)) ([0eab636](0eab636)) * Warehouse redesign part1 ([#2864](#2864)) ([6664457](6664457)) * Warehouse redesign part2 ([#2887](#2887)) ([1aaf417](1aaf417)) * Warehouse redesign part3 ([#2890](#2890)) ([873a1ed](873a1ed)) * Warehouse redesign part4 ([#2893](#2893)) ([d525fd9](d525fd9)) ### 🔧 **Misc** * Add documentation on unset and defaults ([#2882](#2882)) ([85a7836](85a7836)) * apply minor database changes ([#2872](#2872)) ([6ccac59](6ccac59)) * Apply new resource conventions to scim integration ([#2891](#2891)) ([e11e608](e11e608)) * Improve generator template organization ([#2820](#2820)) ([5035e2f](5035e2f)) * Nuke stale objects ([#2869](#2869)) ([9c4a117](9c4a117)) * Show a possible solution for [#2877](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2877) ([#2878](#2878)) ([6fb437b](6fb437b)) * Validations cleanup and old grants removal ([#2884](#2884)) ([05b7eee](05b7eee)) ### 🐛 **Bug fixes:** * Add disclaimers and fix tests ([#2905](#2905)) ([1deaedc](1deaedc)) * Fix cortex search service ([#2904](#2904)) ([763d06c](763d06c)) * use suppressQuoting to fix stage file_format permadiff ([#2885](#2885)) ([fd70f6e](fd70f6e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>
Hey @sfc-gh-mloring @chris-codaio. We have released v0.93.0 yesterday containing cortex search service resource and datasource. Thanks for the contribution one more time @chris-codaio! |
Just back from vacation and looking at this again. Looks like we still can't use this end to end given the following query doesn't work, seemingly necessary to use the
@sfc-gh-mloring - is that something you can help with? |
Also, looks like we have a bug somewhere in trying to pass in that attribute set(string).
|
Fix is already landed and rolling out with 8.27 in the next few days. |
Fixes the issue pointed out in #2860 (comment) Verified in my local deployment PTAL @sfc-gh-asawicki @sfc-gh-mloring
Adds resource & datasource for Cortex search service objects.
Test Plan
PTAL @sfc-gh-mloring
Currently blocked on what looks like a bug in the SQL create flow where the following occurs.