-
Notifications
You must be signed in to change notification settings - Fork 322
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
chore: app for warehouse #3862
chore: app for warehouse #3862
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3862 +/- ##
==========================================
+ Coverage 68.96% 69.19% +0.23%
==========================================
Files 357 357
Lines 53082 53132 +50
==========================================
+ Hits 36607 36764 +157
+ Misses 14155 14051 -104
+ Partials 2320 2317 -3
☔ View full report in Codecov by Sentry. |
a770353
to
10ee94d
Compare
if dbHandle == nil && !isStandAloneSlave(mode) { | ||
return errors.New("warehouse service cannot start, database connection is not setup") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably skip starting in case rudder-core is in degraded mode as we are already skipping the start for the warehouse service later in here.
5c6c904
to
27e3360
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work so far @achettyiitr, it's looking good 👍
Yeah, we can remove this and just pass the dependency for |
Thanks, @fracasula, for pointing it out regarding |
…rver into chore.app-warehouse
40c831e
to
0fc24f7
Compare
…rver into chore.app-warehouse
…rver into chore.app-warehouse
@achettyiitr this could be a good chance to fix deprecated usages of the config api in warehouse, e.g. All other packages have been adapted except for |
Picking it up just after these changes get merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with the last batch of minor comments. Thanks @achettyiitr for the effort! This is good work 👍
3825375
to
7b0ef4e
Compare
7b0ef4e
to
f0e80e0
Compare
Description
Linear Ticket
Security