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

Redshift Connector #1985

Merged
merged 7 commits into from
Sep 2, 2021
Merged

Redshift Connector #1985

merged 7 commits into from
Sep 2, 2021

Conversation

kustosz
Copy link
Contributor

@kustosz kustosz commented Sep 1, 2021

Pull Request Description

Adds DB connector for redshift.

Important Notes

This is not automatically tested. A ticket for tests is #1984

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

@kustosz kustosz added Type: Enhancement p-highest Should be completed ASAP labels Sep 1, 2021
@kustosz kustosz self-assigned this Sep 1, 2021
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Code looks good, but there is an issue with license review that needs to be fixed.

@@ -0,0 +1 @@
regarding copyright ownership. The ASF licenses this file
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong. It is some random line out of context.

It was probably saved using "Keep" while "Keep with context" should have been used. In some situations "Keep in context" cannot be used, so you need to do "Ignore" and manually add the text including all relevant context to copyright-add file. As explained in the license review process docs:
image

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks good to me, assuming that the test suite has at least been ran locally with success.

@kustosz kustosz merged commit b73e5e8 into main Sep 2, 2021
@kustosz kustosz deleted the wip/mk/redshift branch September 2, 2021 09:28
iamrecursion pushed a commit that referenced this pull request Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-highest Should be completed ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants