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 firebird query runner #6730

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

dvandonkelaar
Copy link
Contributor

What type of PR is this?

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

Support of the Firebird database

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Related Tickets & Documents

N/A

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

N/A

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: 46 lines in your changes are missing coverage. Please review.

Comparison is base (97db492) 63.37% compared to head (49a17bf) 63.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6730      +/-   ##
==========================================
- Coverage   63.37%   63.23%   -0.15%     
==========================================
  Files         162      163       +1     
  Lines       13170    13244      +74     
  Branches     1819     1830      +11     
==========================================
+ Hits         8347     8375      +28     
- Misses       4532     4578      +46     
  Partials      291      291              
Files Coverage Δ
redash/settings/__init__.py 98.65% <ø> (ø)
redash/query_runner/firebird.py 37.83% <37.83%> (ø)

Copy link
Collaborator

@guidopetri guidopetri left a comment

Choose a reason for hiding this comment

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

@dvandonkelaar thanks for the PR! It looks like we need some tests to pass codecov - would you mind adding some in? We prefer pytest, but if you're more comfortable with unittest that also works.

Comment on lines +79 to +91
&& rm -rf /tmp/simba; fi \
&& curl "https://github.com/FirebirdSQL/firebird/releases/download/v5.0.0/Firebird-5.0.0.1306-0-linux-x64.tar.gz" --location --output /tmp/firebird.tar.gz \
&& mkdir /tmp/firebird \
&& tar -xvf /tmp/firebird.tar.gz --directory /tmp/firebird \
&& mkdir /tmp/firebird/buildroot \
&& tar -xvf /tmp/firebird/Firebird-*/buildroot.tar.gz --directory /tmp/firebird/buildroot \
&& cp /tmp/firebird/buildroot/opt/firebird/lib/libfbclient.so /usr/lib/libfbclient.so.2 \
&& ln -s /usr/lib/libfbclient.so.2 /usr/lib/libfbclient.so \
&& cp /tmp/firebird/buildroot/opt/firebird/lib/libtomcrypt.so /usr/lib/libtomcrypt.so.1 \
&& ln -s /usr/lib/libtomcrypt.so.1 /usr/lib/libtomcrypt.so \
&& rm /tmp/firebird.tar.gz \
&& rm -rf /tmp/firebird \
&& ln -s /usr/lib/x86_64-linux-gnu/libtommath.so.1 /usr/lib/x86_64-linux-gnu/libtommath.so.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this really need to be installed for a firebird connector to work? I'd imagine we can just ping an existing firebird db to query it - I don't think we'd have to actually install it in the redash image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, we need some client libraries to be able to connect to a Firebird database.
See the docs for more info.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it, thanks.

@dvandonkelaar
Copy link
Contributor Author

Building tests to check if we can connect to a Firebird database is unfortunately not possible. Unfortunately, there is no public database available to connect and query.

I agree that we need to test as much as possible, but don't see how we can test this. I looked into the MSSQL query runner as a template for Firebird (because they work sort or less alike) and also did not find any tests (or maybe I am looking in the wrong place?)

@guidopetri
Copy link
Collaborator

Maybe we can mock the connection? I agree we don't have ideal test coverage, but there's really only one way to improve that.

@dvandonkelaar
Copy link
Contributor Author

Yes, I agree. Let's see if we can mock a connection. Maybe we can also cover some other query runners. I will see if I can accomplish this next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants