consolidate CreateOrUpdateAsync params, solve for revive lint violations #5542
Conversation
k8s-ci-robot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
jackfrancis
force-pushed
the
revive-lint-exceptions
branch
from
b6c6b98 to
89adeac
Compare
| c.Get(gomockinternal.AContext(), gomock.AssignableToTypeOf(azureResourceGetterType)).Return(nil, &azcore.ResponseError{StatusCode: http.StatusNotFound}), | ||
| r.Parameters(gomockinternal.AContext(), nil).Return(fakeParameters, nil), | ||
| c.CreateOrUpdateAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(azureResourceGetterType), "", gomock.Any()).Return(nil, fakePoller[MockCreator](g, http.StatusAccepted), context.DeadlineExceeded), | ||
| c.CreateOrUpdateAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(azureResourceGetterType), gomock.Any()).Return(nil, fakePoller[MockCreator](g, http.StatusAccepted), context.DeadlineExceeded), |
There was a problem hiding this comment.
@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.CreateOrUpdateAsync and it was complaining:
Got: { {<nil> <nil> <nil> <nil> <nil> <nil> <nil> <nil> map[] <nil> <nil> <nil>}} (azure.CreateOrUpdateAsyncOpts)
Want: is equal to { is anything} (azure.CreateOrUpdateAsyncOpts)
Using gomock.Any() to represent the now-current azure.CreateOrUpdateAsyncOpts type seems like cheating...
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5542 +/- ##
==========================================
- Coverage 52.93% 52.92% -0.01%
==========================================
Files 272 272
Lines 29485 29484 -1
==========================================
- Hits 15607 15604 -3
- Misses 13061 13063 +2
Partials 817 817 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Jack Francis <[email protected]>
jackfrancis
force-pushed
the
revive-lint-exceptions
branch
from
89adeac to
7ee0380
Compare
| // 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.
That this is called CreateOrUpdateAsync but isn't actually async at all is the bigger smell to me than an unused parameter. Passing both parameters in a single struct and then only using one field of the struct is just sweeping the problem under the rug IMO.
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.
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.
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 _ and remove the //nolints. Anything more I think isn't worth sacrificing how all the SDK services are currently set up more or less the same way.
make networkinterfaces.AzureClient exportable so that it can be hygienically used as a return value for the exported NewClient func Signed-off-by: Jack Francis <[email protected]>
jackfrancis
force-pushed
the
revive-lint-exceptions
branch
from
7ee0380 to
10c1fe6
Compare
|
use #5543 instead |
3 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR updates the
async.CreateOrUpdateAsyncmethod signature so that it takes a single strongly typed structure to express various configurations, to accommodate for the fact that across the various implementations of this interface we use a combination of those configurations. This makes it easier to implement the interface method in a variety of ways without having to ignore the parameter input variable (_), or declaring a function parameter variable + not using it, and then declaring//nolint:reviveto avoid a source code lint violation.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
TODOs:
Release note: