redash icon indicating copy to clipboard operation
redash copied to clipboard

Sqlalchemy 2.0

Open AndrewChubatiuk opened this issue 1 year ago • 22 comments

What type of PR is this?

Upgraded SQLAlchemy 1.3 -> 2.0

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

How is this tested?

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

AndrewChubatiuk avatar Dec 21 '23 19:12 AndrewChubatiuk

@guidopetri @konnectr If it is okay to merge this from your perspective, please let me know. If so, I will consider this again.

masayuki038 avatar Dec 22 '23 09:12 masayuki038

@masayuki038 besides:

  • removing -p option in ci
  • moving Alerts states to a separate Enum

other changes were required:

  • paginator changes were needed cause paginator function from flask-sqlachemy has changed
  • removing relative imports was related to issues in sqlalchemy
  • sort_query function was completely removed from sqlalchemy-utils, so it had to be replaced
  • all json changes were related to a bunch of issues in tests with a filtering in where clause

sqlalchemy dependencies introduce lots of changes, which do not allow to prepare tiny diffs. god bless flask and all its dependencies was not required to be upgraded together with sqlalchemy deps

AndrewChubatiuk avatar Dec 22 '23 10:12 AndrewChubatiuk

@masayuki038 could you please trigger ci from the latest commit, I've fixed lint there

AndrewChubatiuk avatar Dec 22 '23 10:12 AndrewChubatiuk

It looks ci is running now... https://github.com/getredash/redash/actions/runs/7298330872

masayuki038 avatar Dec 22 '23 12:12 masayuki038

It looks ci is running now... https://github.com/getredash/redash/actions/runs/7298330872

build for a latest commit was not triggered

AndrewChubatiuk avatar Dec 22 '23 15:12 AndrewChubatiuk

@AndrewChubatiuk I manually re-run Github Action, but I may not have been able to fetch the latest code. I tried deleting my review, but the status did not change.

Could you try pushing something? Or could you please close this and create another PR?

masayuki038 avatar Dec 23 '23 02:12 masayuki038

@masayuki038 please approve this pipeline https://github.com/getredash/redash/actions/runs/7307066364

AndrewChubatiuk avatar Dec 23 '23 08:12 AndrewChubatiuk

@masayuki038 some changes were lost during rebase. pushed once again

AndrewChubatiuk avatar Dec 23 '23 09:12 AndrewChubatiuk

Codecov Report

Attention: Patch coverage is 87.09677% with 56 lines in your changes are missing coverage. Please review.

Project coverage is 63.90%. Comparing base (b7f22b1) to head (05bbe48). Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6671      +/-   ##
==========================================
+ Coverage   63.80%   63.90%   +0.09%     
==========================================
  Files         161      162       +1     
  Lines       13087    13175      +88     
  Branches     1812     1824      +12     
==========================================
+ Hits         8350     8419      +69     
- Misses       4434     4446      +12     
- Partials      303      310       +7     
Files Coverage Δ
redash/alerts.py 100.00% <100.00%> (ø)
redash/app.py 100.00% <100.00%> (ø)
redash/authentication/__init__.py 81.44% <100.00%> (+0.09%) :arrow_up:
redash/cli/organization.py 91.66% <100.00%> (+0.23%) :arrow_up:
redash/destinations/asana.py 81.48% <100.00%> (ø)
redash/handlers/alerts.py 86.74% <100.00%> (+0.16%) :arrow_up:
redash/handlers/data_sources.py 72.05% <100.00%> (ø)
redash/handlers/destinations.py 77.92% <100.00%> (ø)
redash/handlers/embed.py 87.50% <100.00%> (ø)
redash/handlers/organization.py 81.81% <100.00%> (+1.81%) :arrow_up:
... and 41 more

codecov[bot] avatar Dec 23 '23 11:12 codecov[bot]

I understand correctly that we are still abandoning simplejson ?

konnectr avatar Dec 24 '23 04:12 konnectr

I will try to test the operation in manual mode in the near future. It is unlikely that our auto-tests cover all possible problems

konnectr avatar Dec 24 '23 04:12 konnectr

@konnectr Thanks! I will start to review this next week as well.

masayuki038 avatar Dec 24 '23 07:12 masayuki038

@masayuki038 @justinclift is anyone able to review it? I've done all requested changes and answered all questions

AndrewChubatiuk avatar Apr 27 '24 15:04 AndrewChubatiuk

@AndrewChubatiuk Thanks for your updating. I will check it.

masayuki038 avatar Apr 27 '24 15:04 masayuki038

@AndrewChubatiuk I checked and commented. Could you check them?

masayuki038 avatar Apr 28 '24 16:04 masayuki038

thanks @masayuki038, made updates, please check again

AndrewChubatiuk avatar Apr 28 '24 19:04 AndrewChubatiuk

@AndrewChubatiuk I checked and marked "Resolved" in some of them. And I left some comments. Could you check them?

masayuki038 avatar Apr 29 '24 14:04 masayuki038

@masayuki038 already checked them. answered or made code changes

AndrewChubatiuk avatar Apr 29 '24 15:04 AndrewChubatiuk

@AndrewChubatiuk Could you stop to update the things other than "Sqlalchemy 2.0"?

masayuki038 avatar May 02 '24 12:05 masayuki038

@AndrewChubatiuk Could you stop to update the things other than "Sqlalchemy 2.0"?

@masayuki038 i update only things, which are impacted by pr comments changes

AndrewChubatiuk avatar May 02 '24 12:05 AndrewChubatiuk

@AndrewChubatiuk OK. But, I don't think we need to update "assert_called_once_with("requests.redash_ping.get", ANY)" to "assert_called" like the above thread by upgrading to SQLAlchemy 2.0... Why did you update it yesterday?

masayuki038 avatar May 02 '24 12:05 masayuki038

@AndrewChubatiuk OK. But, I don't think we need to update "assert_called_once_with("requests.redash_ping.get", ANY)" to "assert_called" like the above thread by upgrading to SQLAlchemy 2.0... Why did you update it yesterday?

replied in a thread

AndrewChubatiuk avatar May 02 '24 12:05 AndrewChubatiuk