fix: removing the backup bucket env check to determine if backup is enabled #2668
Conversation
|
@BonapartePC is this change needed for |
Made the change |
| @@ -29,7 +29,7 @@ type backupSettings struct { | |||
| } | |||
|
|
|||
| func (b *backupSettings) isBackupEnabled() bool { | |||
| return masterBackupEnabled && b.instanceBackupEnabled && config.GetString("JOBS_BACKUP_BUCKET", "") != "" | |||
There was a problem hiding this comment.
don't we need a default (fallback) backup bucket anyway? I don't think it is safe to remove this condition for single tenant deployments
There was a problem hiding this comment.
True, but checking if JOBS_BACKUP_BUCKET env is set to determine if the backup is enabled does not seem right since we can take bucket details from the user as well right?
There was a problem hiding this comment.
Then jobsdb needs to consult another component for that?
There was a problem hiding this comment.
No, jobsdb only cares if master backup is enabled and instance backup is enabled and it's the job of fileuploader to provide bucket details to it right?
There was a problem hiding this comment.
we can take bucket details from the user as well right?
But still, don't we need to have a default bucket properly setup in a multitenant deployment, regardless of workspace-specific configurations?
There was a problem hiding this comment.
True, that assertion should happen independently of this right? Probably somewhere even before
There was a problem hiding this comment.
Probably check if all the backup related env are set?
There was a problem hiding this comment.
I agree with @atzoum. It is safer to keep the env check to ensure a default bucket is set for fallback, until we have a better way to do it.
Codecov ReportBase: 45.60% // Head: 45.45% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## release/1.3.x #2668 +/- ##
=================================================
- Coverage 45.60% 45.45% -0.15%
=================================================
Files 287 287
Lines 47790 47777 -13
=================================================
- Hits 21793 21717 -76
- Misses 24619 24683 +64
+ Partials 1378 1377 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Description
Removing the backup bucket env check to determine if backup is enabled since bucket can be provided by customer as well
Notion Ticket
< Replace with Notion Link >
Security