-
Notifications
You must be signed in to change notification settings - Fork 460
consolidate CreateOrUpdateAsync params, solve for revive lint violations #5542
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,13 +60,13 @@ | |
| // CreateOrUpdateAsync creates or updates an availability set asynchronously. | ||
| // It sends a PUT request to Azure and if accepted without error, the func will return a Poller which can be used to track the ongoing | ||
| // progress of the operation. | ||
| func (ac *AzureClient) CreateOrUpdateAsync(ctx context.Context, spec azure.ResourceSpecGetter, _resumeToken string, parameters interface{}) (result interface{}, poller *runtime.Poller[armcompute.AvailabilitySetsClientCreateOrUpdateResponse], err error) { //nolint:revive // keeping _resumeToken for understanding purposes | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That this is called I get that the SDK simply doesn't give async variants for some types so this is the lowest common denominator, but a more "proper" fix to me would be to have some way to classify services as either "async" or "not async" so the "not async" services like this one are never even passed a resume token at all, not even an empty placeholder. Or do a wholesale rewrite of all the service clients to be pure generic ARM like ASO where everything really is handled the same way. This may be one reason to prefer ASO long-term. K8s resources defined by ASO are overall way more homogenous than SDK objects. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverting this probably wouldn't be that much work: Or we could wrap the clients that don't have native asynchronous SDK interfaces in our own async business logic. In any event these are all good considerations for how to proceed going forward. Calculated using a conventional effort * return formula. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess I can convince myself that the "Async" part of "CreateOrUpdateAsync" describes CAPZ's higher-level behavior rather than reflecting exactly what's being done here, so I'm not too hung up on the name. Overall I think if we do anything here for now we change the parameters to |
||
| func (ac *AzureClient) CreateOrUpdateAsync(ctx context.Context, spec azure.ResourceSpecGetter, opts azure.CreateOrUpdateAsyncOpts) (result interface{}, poller *runtime.Poller[armcompute.AvailabilitySetsClientCreateOrUpdateResponse], err error) { | ||
| ctx, _, done := tele.StartSpanWithLogger(ctx, "availabilitySets.AzureClient.CreateOrUpdateAsync") | ||
| defer done() | ||
|
|
||
| availabilitySet, ok := parameters.(armcompute.AvailabilitySet) | ||
| if !ok && parameters != nil { | ||
| return nil, nil, errors.Errorf("%T is not an armcompute.AvailabilitySet", parameters) | ||
| availabilitySet, ok := opts.Parameters.(armcompute.AvailabilitySet) | ||
| if !ok && opts.Parameters != nil { | ||
| return nil, nil, errors.Errorf("%T is not an armcompute.AvailabilitySet", opts.Parameters) | ||
| } | ||
|
|
||
| // Note: there is no async `BeginCreateOrUpdate` implementation for availability sets, so this func will never return a poller. | ||
|
|
@@ -82,7 +82,7 @@ | |
| // DeleteAsync deletes a availability set asynchronously. DeleteAsync sends a DELETE | ||
| // request to Azure and if accepted without error, the func will return a Poller which can be used to track the ongoing | ||
| // progress of the operation. | ||
| func (ac *AzureClient) DeleteAsync(ctx context.Context, spec azure.ResourceSpecGetter, _resumeToken string) (poller *runtime.Poller[armcompute.AvailabilitySetsClientDeleteResponse], err error) { //nolint:revive // keeping _resumeToken for understanding purposes | ||
| func (ac *AzureClient) DeleteAsync(ctx context.Context, spec azure.ResourceSpecGetter, _ string) (poller *runtime.Poller[armcompute.AvailabilitySetsClientDeleteResponse], err error) { | ||
| ctx, _, done := tele.StartSpanWithLogger(ctx, "availabilitysets.AzureClient.DeleteAsync") | ||
| defer done() | ||
|
|
||
|
|
||
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.
@nojnhuh this is the only way I could get this test to pass, I tried using:
azure.CreateOrUpdateAsyncOpts{ResumeToken: "", Parameters: gomock.Any()}as the 3rd argument to
c.CreateOrUpdateAsyncand it was complaining:Using
gomock.Any()to represent the now-currentazure.CreateOrUpdateAsyncOptstype seems like cheating...