[fix] cluster_name = cluster_name, not cluster_id #8514
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8514 +/- ##
=======================================
Coverage 66.68% 66.68%
=======================================
Files 449 449
Lines 22684 22684
Branches 2366 2366
=======================================
Hits 15126 15126
Misses 7420 7420
Partials 138 138
Continue to review full report at Codecov.
|
| @@ -1612,7 +1612,7 @@ def query_datasources_by_name( | |||
| ) -> List["DruidDatasource"]: | |||
| return ( | |||
| session.query(cls) | |||
| .filter_by(cluster_name=database.id) | |||
| .filter_by(cluster_name=database.database_name) | |||
There was a problem hiding this comment.
@michellethomas
This didn't cause any problems for us because we use our own security manager, so this function isn't used.
I did a little more digging, and I'm not entirely certain that cluster_name=database.database_name is correct, since clusters != databases in our tables. Maybe this line should just be removed? Do you know anyone with more context on this kind of thing? Either way, this change won't cause any (more) problems, but it does seem wrong.
There was a problem hiding this comment.
Maybe @mistercrunch could chime in here? This code is pretty old
There was a problem hiding this comment.
Mmmh, for reference, this came from #2497.
Clearly there are issues around the Connector interface. Good news is we're planning on deprecating the Druid connector in favor of SQLAlchemy
There was a problem hiding this comment.
This new line is likely to fail as much as the previous line.
CATEGORY
Choose one
SUMMARY
Found these 3-year-old bugs while looking into some other stuff. I'm guessing this may have happened because of some copy paste errors. It probably isn't too important since they haven't been caught, but it's pretty irksome to look at, so here we go.
TEST PLAN
Passes
tox.REVIEWERS
@john-bodley