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

Add support for snowflake tests. #665

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

nickpileggi947
Copy link
Contributor

Add in support for snowflake DB. Integration tests are not coded since there is no easy to set up a free repeatable way for someone to test.

This resolves issue #642

@nickpileggi947
Copy link
Contributor Author

@javornikolov Is there anything I need to change for this PR?

@javornikolov
Copy link
Contributor

@javornikolov Is there anything I need to change for this PR?

Hi @nickpileggi947, thanks for your contribution! I need to look closer. One thing at first glimpse is that custom_libs content is supposed to be empty in the repo (it's intended for local tweaking). If a driver is not available in public artifact repo, we can upload it to our dedicated s3 place (I can take care of that).
Also - I am not sure we really need to change a Fitnesse properties file (I saw it in one of the commits).

@nickpileggi947
Copy link
Contributor Author

@javornikolov Appreciate the first set of feedback, I have pushed another commit that should fix those. Let me know if there is anything else you see that needs to be updated.

@nickpileggi947
Copy link
Contributor Author

@javornikolov just checking if there was anything else I needed to change on this PR?

@nickpileggi947
Copy link
Contributor Author

@javornikolov Just checking in again to see if you (or any other approver) is happy with this PR?

@nickpileggi947
Copy link
Contributor Author

@javornikolov All of the reviews have been closed and updated.

@javornikolov
Copy link
Contributor

@nickpileggi947, I added a few comments which are relatively small. The rest looks good, a few outstanding things:

  • May we squash the multiple commits into a single or few (we don't need the history of all small fixups).
  • I wonder if it would be possible to somehow run snowflake integration tests on TravisCI. (We might postpone that if it's a pain)

@nickpileggi947
Copy link
Contributor Author

@javornikolov I'll review and update these later today. But yea, we can squash these down to one large commit. And I would love to run the snowflake integration tests on TravisCI, but there is no free way to get a DB instance.

@nickpileggi947
Copy link
Contributor Author

@javornikolov Sorry for the late response, I have made all of the updates and squashed the commits to one. Let me know if you need anything else.

@javornikolov
Copy link
Contributor

@javornikolov Sorry for the late response, I have made all of the updates and squashed the commits to one. Let me know if you need anything else.

Thank you @nickpileggi947! Looks good to me 👍 I am merging it.

@javornikolov javornikolov added this to the 4.0.0 milestone Mar 12, 2021
@javornikolov javornikolov merged commit 60db821 into dbfit:master Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants