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

feat: Stream on external table resource #3122

Merged
merged 5 commits into from
Oct 10, 2024
Merged

feat: Stream on external table resource #3122

merged 5 commits into from
Oct 10, 2024

Conversation

sfc-gh-jmichalak
Copy link
Collaborator

@sfc-gh-jmichalak sfc-gh-jmichalak commented Oct 9, 2024 •
edited
Loading

  • add snowflake_stream_on_external_table resource
  • adjust copy grants documentation
  • adjust external table helper client
  • move copy grants handling to resource_helpers_create.go
  • move baseModel to ext
  • move common stream code to stream_common.go
  • fix using empty columns in views recreation

Test Plan

  • acceptance tests

References

https://docs.snowflake.com/en/sql-reference/sql/create-stream

#3073

TODO

  • add remaining resources (stage, view)
  • rework data source

@sfc-gh-jmichalak sfc-gh-jmichalak marked this pull request as ready for review October 9, 2024 12:08
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 9, 2024

Integration tests failure for 62699dfcf52ba6a79d3cffda4dd3676947217df0

sfc-gh-asawicki
sfc-gh-asawicki previously approved these changes Oct 9, 2024
View reviewed changes
pkg/resources/stream_common.go
stream *sdk.Stream,
streamDescription *sdk.Stream,
) error {
if err := d.Set("comment", stream.Comment); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we could use errors.Join(...) and return just one error here. Maybe it does not help that much with only 4 params but at least we return all the errors instead of just the first encountered one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll do this in the next PR.

pkg/resources/stream_on_external_table.go
if err != nil {
return nil, err
}
if err := d.Set("name", id.Name()); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: these could be extracted because they set a few common stream fields

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll do this in the next PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

also depending on the importance of insert_only here the whole import could be entirely replaced by ImportName function

pkg/resources/view_acceptance_test.go
@@ -923,6 +923,63 @@ func TestAcc_View_Issue3073(t *testing.T) {
})
}

func TestAcc_View_IncorrectColumnsWithOrReplace(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can you please reference the GH issue here, it helps to give context when someone will be studying these tests later not from the PR context

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll do this in the next PR.

sfc-gh-jcieslak
sfc-gh-jcieslak reviewed Oct 10, 2024
View reviewed changes
pkg/resources/stream_common.go Show resolved Hide resolved
pkg/resources/stream_on_external_table.go
if err != nil {
return nil, err
}
if err := d.Set("name", id.Name()); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

also depending on the importance of insert_only here the whole import could be entirely replaced by ImportName function

pkg/resources/stream_on_table_acceptance_test.go Show resolved Hide resolved
pkg/resources/stream_on_external_table_acceptance_test.go Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

Integration tests failure for 41d3a43dada3cef3705cb1cf7a2ebe85d1b98aaf

@github-actions GitHub Actions
Copy link

Integration tests cancelled for 63d8e9ff3164f7c04f2d5ede549bb132279bea7a

@sfc-gh-jmichalak sfc-gh-jmichalak merged commit d837341 into main Oct 10, 2024
8 of 9 checks passed
@sfc-gh-jmichalak sfc-gh-jmichalak deleted the stream-v1 branch October 10, 2024 13:23
@snowflake-release-please snowflake-release-please bot mentioned this pull request Oct 10, 2024
Merged
@github-actions GitHub Actions
Copy link

Integration tests cancelled for 454bebaa67c3229103ce189c2fec003ced1c47b0

sfc-gh-jcieslak pushed a commit that referenced this pull request Oct 11, 2024
@snowflake-release-please
chore(main): release 0.97.0 (#3100)
561f78e 
🤖 I have created a release *beep* *boop*
---


##
[0.97.0](v0.96.0...v0.97.0)
(2024-10-10)


### 🎉 **What's new:**

* Add secret to sdk
([#3091](#3091))
([7430aee](7430aee))
* Add service user and legacy service user resources
([#3119](#3119))
([0e88e08](0e88e08))
* Handle all Task parameters in SDK
([#3103](#3103))
([08ae072](08ae072))
* Stream on external table resource
([#3122](#3122))
([d837341](d837341))
* Stream on table resource
([#3109](#3109))
([97fa9b4](97fa9b4))
* Tasks v1 readiness - SDK part
([#3086](#3086))
([0a77383](0a77383))
* Upgrade stream sdk
([#3105](#3105))
([ad5fa11](ad5fa11))


### 🔧 **Misc**

* Add pre check to each datasource
([#3065](#3065))
([560ab6b](560ab6b))
* Bump golang-ci lint to 1.61
([#3112](#3112))
([f23e085](f23e085))
* Secret Validation change
([#3111](#3111))
([666630e](666630e))


### 🐛 **Bug fixes:**

* Fix parsing text in view, check parenthesis in
ParseSchemaObjectIdentifierWithArguments
([#3102](#3102))
([b0a67e6](b0a67e6))
* Try to reproduce 2679 and 3117
([#3124](#3124))
([ccdbc30](ccdbc30))

---
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>
@sfc-gh-jmichalak sfc-gh-jmichalak mentioned this pull request Oct 11, 2024
Merged
2 tasks
sfc-gh-jmichalak added a commit that referenced this pull request Oct 22, 2024
@sfc-gh-jmichalak
feat: Add stream on directory table (#3129)
4391473 
<!-- Feel free to delete comments as you fill this in -->
- add new `snowflake_stream_on_directory_table` resource
- adjust SDK to handle non-qualified names for streams' tables
- address open comments from the previous
[pr](#3122)
- recreate streams when they are stale (ref
#1150)
- change diff suppress on copy_grants
- improve procedure grants documentation (ref
#3139)
<!-- summary of changes -->

## Test Plan
<!-- detail ways in which this PR has been tested or needs to be tested
-->
* [x] acceptance tests
<!-- add more below if you think they are relevant -->
* [ ] …

## References
<!-- issues documentation links, etc  -->
https://docs.snowflake.com/en/sql-reference/sql/create-stream

## TODO
- add streams on views
- add streams data source
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

@sfc-gh-jcieslak sfc-gh-jcieslak sfc-gh-jcieslak 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.
3 participants
@sfc-gh-jmichalak @sfc-gh-jcieslak @sfc-gh-asawicki