chore: pass context

[achettyiitr]

Description

• Pass context from UploadJob down to the Integrations package. One context is created during the UploadJob and passed down to the integrations.

Notion Ticket

https://www.notion.so/rudderstacks/make-sure-context-works-as-intended-1d128f7c18fa49acaf007ab82b20394b?pvs=4

Security

• The code changed/added as part of this pull request won't create any security issues with how the software is being used.

[codecov]

Codecov Report

Patch coverage: 79.27% and project coverage change: +0.36 🎉

Comparison is base (4a94c15) 68.12% compared to head (1dd32b2) 68.48%.

❗ Current head 1dd32b2 differs from pull request most recent head 25a24d8. Consider uploading reports for the commit 25a24d8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3326      +/-   ##
==========================================
+ Coverage   68.12%   68.48%   +0.36%     
==========================================
  Files         329      329              
  Lines       53492    52710     -782     
==========================================
- Hits        36443    36101     -342     
+ Misses      14719    14270     -449     
- Partials     2330     2339       +9     

Impacted Files	Coverage Δ
warehouse/admin.go	3.17% <0.00%> (ø)
warehouse/errors.go	89.47% <ø> (+3.75%)	⬆️
...ns/datalake/schema-repository/schema_repository.go	85.00% <ø> (ø)
warehouse/integrations/manager/manager.go	95.83% <ø> (ø)
warehouse/jobs/jobs.go	7.69% <0.00%> (ø)
warehouse/utils/utils.go	82.27% <ø> (ø)
warehouse/client/client.go	48.31% <33.33%> (ø)
warehouse/identities.go	19.40% <43.75%> (+2.55%)	⬆️
warehouse/integrations/datalake/datalake.go	60.00% <47.61%> (ø)
warehouse/integrations/mssql/mssql.go	64.07% <56.60%> (-0.06%)	⬇️
... and 29 more

... and 15 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[fracasula]

The context seems to be passed to backendConfig.WaitForConfig() only. What if it gets canceled or it times out though? Do we want GetProviderConfigFromEnv to return an error?

[cisse21]

Do we even need a context propagated to this function?

[fracasula]

I guess we do, in case of cancelation it would stop waiting for the backend config which is a blocking call, right?

[cisse21]

Yeah true but that part of code kinda feels weird to me and probably something which no one would ever use.

Because from the UI he won't even be able to create a S3 destination without filling the required fields but we are waiting for config to get the workspaceID for a destination which isn't there. Not sure if I put it out here properly

[fracasula]

@cisse21 I think this is about proper context propagation thus implicitly honouring such contexts. As far as I understand these calls would be part of the gRPC server. So what if the client disconnects during a gRPC request? I would expect the context to get canceled. In such scenarios, with proper ctx propagation, the backendConfig.WaitForConfig would correctly return immediately instead of blocking and creating a (temporary) leak:

		pkgLogger.Info("Waiting for backend config")
		select {
		case <-ctx.Done():
			return

So all considered, I think context propagation does make sense. What I'm not sure of is whether it makes sense to have the function return an error too so that it would properly be logged and handled upstream i.e.:

func overrideWithEnv(ctx context.Context, settings *filemanager.SettingsT) error {
    // ...

    return ctx.Err()
}

If yes then getFileManagerSettings can return an error as well which can be used by validateObjectStorage which already returns an error that is handled upstream.

@achettyiitr any thoughts as well?

[achettyiitr]

@fracasula I am also aligned with passing the context.