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

fix: file_format validate() for field_optionally_enclosed_by #2575

Merged
merged 12 commits into from
Mar 5, 2024
Merged

fix: file_format validate() for field_optionally_enclosed_by #2575

merged 12 commits into from
Mar 5, 2024

Conversation

raulbonet
Copy link
Contributor

ref #2385

As pointed out by https://github.com/alexmaras , the validate() function does not properly validate the use-case where field_optionally_enclosed_by is None.

@raulbonet raulbonet changed the title Feature/2385/file format bug fix: file_format validate() for field_optionally_enclosed_by Feb 29, 2024
@raulbonet
Copy link
Contributor Author

@sfc-gh-asawicki
I wonder if the validation() function should go as far as validating potential values that Snowflake will already validate when the query is executed. Not sure if it would be best just to remove all together this part of the validate. If tomorrow Snowflake adds other accepted values to this list, it would manually need to be modified.

@sfc-gh-asawicki
Copy link
Collaborator

@sfc-gh-asawicki I wonder if the validation() function should go as far as validating potential values that Snowflake will already validate when the query is executed. Not sure if it would be best just to remove all together this part of the validate. If tomorrow Snowflake adds other accepted values to this list, it would manually need to be modified.

We currently have this discussion when approaching resources redesign. Our SDK-level validations or sometimes to strict and it opens us to the problem you mentioned. We will probably soften the validations on the general level but there is no decision yet.

@sfc-gh-asawicki
Copy link
Collaborator

Thanks for the PR @raulbonet. I left one comment.

@sfc-gh-asawicki sfc-gh-asawicki self-requested a review March 4, 2024 09:34
@sfc-gh-asawicki
Copy link
Collaborator

/ok-to-test sha=175d5d28bd32923e183e4c5ff31fbb86a7d96559

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 5, 2024

Integration tests failure for 175d5d28bd32923e183e4c5ff31fbb86a7d96559

@sfc-gh-asawicki
Copy link
Collaborator

/ok-to-test sha=6019e8b3e2b92a36cebc8cb8e034daaebbfd4300

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 5, 2024

Integration tests success for 6019e8b3e2b92a36cebc8cb8e034daaebbfd4300

@sfc-gh-asawicki sfc-gh-asawicki merged commit 5da5d93 into Snowflake-Labs:main Mar 5, 2024
8 checks passed
@snowflake-release-please snowflake-release-please bot mentioned this pull request Mar 5, 2024
Merged
@sfc-gh-asawicki
Copy link
Collaborator

Thanks for the contribution @raulbonet! 🎉

sfc-gh-jcieslak pushed a commit that referenced this pull request Mar 6, 2024
@snowflake-release-please
chore(main): release 0.87.1 (#2574)
40861fa 
🤖 I have created a release *beep* *boop*
---


##
[0.87.1](v0.87.0...v0.87.1)
(2024-03-06)


### 🔧 **Misc**

* Add deprecated resources/datasources to the main documentation page
([#2581](#2581))
([68bbf4f](68bbf4f))
* Add deprecation message to docs
([#2578](#2578))
([3675d6d](3675d6d))
* Add Terraform setup to the testing pipeline
([#2579](#2579))
([216e35a](216e35a))
* Chore use get or skip in other tests
([#2570](#2570))
([2829b90](2829b90))
* modify provider context to pass snowflake client instead of db
connection
([#2577](#2577))
([e7fd4ef](e7fd4ef))
* Speed up tests
([#2580](#2580))
([f003715](f003715))


### 🐛 **Bug fixes:**

* file_format validate() for field_optionally_enclosed_by
([#2575](#2575))
([5da5d93](5da5d93))
* Fix import for account parameter
([#2594](#2594))
([cac884c](cac884c)),
closes
[#2573](#2573)
* support snowflake application as database
([#2596](#2596))
([b9a4a19](b9a4a19))
* tag identifiers problems
([#2534](#2534))
([3c300e1](3c300e1))

---
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>
@alexmaras alexmaras mentioned this pull request Mar 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-asawicki sfc-gh-asawicki sfc-gh-asawicki approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@raulbonet @sfc-gh-asawicki