-
Notifications
You must be signed in to change notification settings - Fork 27
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
Use service-handler for restore operations #4714
Conversation
Current Aviator status
This PR was merged using Aviator. See the real-time status of this PR on the Aviator webapp. Use the Aviator Chrome Extension to see the status of your PR within GitHub.
|
src/internal/operations/restore.go
Outdated
progressBar := observe.MessageWithCompletion(ctx, observe.ProgressCfg{}, "Restoring data") | ||
defer close(progressBar) | ||
|
||
ctx, end := diagnostics.Span(ctx, "m365:restore") |
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.
this is the wrong layer to specify m365. operations should be agnostic to the provider.
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.
whoops, copy/paste error. I'll switch to something like operations
for this annotation
// TODO(ashmrtn): We should stop relying on the context for rate limiter stuff | ||
// and instead configure this when we make the handler instance. We can't |
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.
Two requests.
1/ can you please make a tech debt ticket for this, instead of todo comments?
2/ while i'm fine that we avoid hijacking the context for this, I worry that the lack of a suggested alternative leaves this open to going back to global singletons as a solution, which I'd like to avoid. We've been pretty loose (ie: not centralized) when it comes to handling our http client configuration, and that's caused a handful of problems around statefulness and leaky configs. It'd be nice for a refactor to look at solving all of that, and I worry a quick todo resolution won't have the necessary breathing room to solve many problems beyond the ctx usage itself.
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.
can we switch up how we do http client init a little? From my understanding, we need the client in the CLI layer to do some name resolution and in the operation itself. Would it be alright to make those two separate clients instead of reusing the same one? That would get us out of needing a global config that's initialized before we get all the info we need
The one question there is if it would scale nicely for something like corso backup create exchange --user "*"
. If we tie the client to the operation (or functions like this) it's unclear how garbage collection/reuse of the old client's connection pool will go
b0bf1dd
to
d20dd1a
Compare
Switch the existing restore functions for each service to be a method of the service handler for the service instead of a free function.
Just making sure everything will continue to compile at the end.
Update where ConsumeRestoreCollections and some other functions are called. Mostly just switching things to make a handler and making return values and parameters line up.
These statements were originally in the internal/m365/restore.go file, but that's going to be removed soon. Move them to operations since it's still a centralized place.
Move check to more centralized place to give same behavior but in a more maintainable manner.
Return error if there's no data given to restore.
4e967a5
to
3a41e36
Compare
Kudos, SonarCloud Quality Gate passed!
|
Switch restore code path to use service-level handlers. Does a few
things:
service-level handlers
the new handlers or the operation-level function
May be easiest to review by commit
Does this PR need a docs update or release note?
Type of change
Issue(s)
Test Plan