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

[feature] storage integration #140

Merged
merged 10 commits into from
Mar 10, 2020
Merged

[feature] storage integration #140

merged 10 commits into from
Mar 10, 2020

Conversation

sjauld
Copy link
Contributor

@sjauld sjauld commented Feb 20, 2020 •
edited
Loading

Ok everything seems to be fixed up and working now.

This adds

  • storage integration resource
  • support to specify a storage integration for a stage

Closes #125
Closes #144

@sjauld sjauld requested a review from a team as a code owner February 20, 2020 03:10
sjauld
sjauld commented Mar 2, 2020
View reviewed changes
Copy link
Contributor Author

@sjauld sjauld left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback!

@sjauld
Copy link
Contributor Author

sjauld commented Mar 3, 2020

Ooh also resolves #144

@ryanking
Copy link
Contributor

ryanking commented Mar 3, 2020

Ooh also resolves #144

you can edit the description above to add that

@brewkode
Copy link

brewkode commented Mar 6, 2020

Whole set of "Acc" related tests have been failing from the first commit after "go fmt" in this branch. @sjauld May be master was not stable when the feature branch fork happened ? I tried merging master into this branch and had lot of conflicts. Would need sometime to understand this fully.

@sjauld
Copy link
Contributor Author

sjauld commented Mar 6, 2020

I don't understand your issue with conflicts- this branch is forked from the current HEAD of master, so there is nothing to merge. Have you tried merging this repo's master into your local master before merging my branch?

Travis tests will not pass on pull requests since they don't get access to snowflake creds. I think we should take an approach like the AWS provider but that's a seperate issue.

Also, I have no idea why the docs tests are failing, they have changed since I last contributed here and I'm not sure what the issue is, but that seems to also affect all PRs.

@brewkode
Copy link

brewkode commented Mar 6, 2020

Thanks for your response @sjauld . I did not spend time on merge failures, Im going to do now and will keep you posted. I did not realize the issue with snowflake creds - my bad!

@brewkode
Copy link

brewkode commented Mar 6, 2020 •
edited
Loading

@ryanking and @alldoami So, do you think, we can merge this PR ?

alldoami
alldoami approved these changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@alldoami alldoami left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for doing this!

@alldoami
Copy link
Contributor

alldoami commented Mar 9, 2020 •
edited
Loading

make check-docs failed so please run make docs and push again :) I'll run the test separately and then merge! @sjauld

alldoami
alldoami suggested changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@alldoami alldoami left a comment

Choose a reason for hiding this comment

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

make check-docs failed so please run make docs and push again :) I'll run the test separately and then merge! @sjauld

@sjauld
Copy link
Contributor Author

sjauld commented Mar 9, 2020

make check-docs failed so please run make docs and push again :) I'll run the test separately and then merge! @sjauld

@alldoami make check-docs is passing now https://travis-ci.com/chanzuckerberg/terraform-provider-snowflake/jobs/296288487

@alldoami
Copy link
Contributor

alldoami commented Mar 10, 2020 •
edited
Loading

Ran make test-acceptance-ci and looked good! Will merge. Thanks so much for working on this @sjauld!

@alldoami alldoami merged commit 17c22dc into Snowflake-Labs:master Mar 10, 2020
@sjauld sjauld deleted the feature/storage-integration branch March 10, 2020 00:36
@yuriolive
Copy link
Contributor

Any plans to have a release with this functionality?

@brewkode
Copy link

@alldoami Thanks for taking a look and merging this PR. @sjauld Thanks for helping with this feature 👍

Can we please 🙏 cut a release with this functionality ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brewkode brewkode brewkode left review comments

@alldoami alldoami alldoami requested changes

Assignees

@sjauld sjauld

@alldoami alldoami

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

snowflake_stage: support for storage_integration Add storage_integration
5 participants
@sjauld @ryanking @brewkode @alldoami @yuriolive