SDK2: Replace old SDK in preview feature controller #3922
Conversation
bitoku
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
cadenmarchese,
UlrichSchlueter,
SudoBrendan,
yjst2012,
jaitaiwan,
anshulvermapatel,
hlipsig,
tiguelu,
SrinivasAtmakuri,
mociarain,
kimorris27,
tsatam and
fahlmant
as code owners
bitoku
force-pushed
the
sdk2/previewfeature_controller
branch
from
f511938
to
f7b208f
Compare
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kimorris27
dismissed
their stale review
Sorry, I forgot I wanted to wait for E2E to pass before approving.
|
/azp run e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The first E2E failure looked like a flake on our end, and the second one looked like a transient Azure API failure (listing VM SKUs in |
|
/azp run e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Third E2E run had the same failure as the first - cluster installation failed because k8s API server wasn't ready. There's no way it's related to your changes, but if it happens again on this fourth try, maybe we should start looking at changes that were merged recently (?). |
|
Please rebase pull request. |
…_controller # Conflicts: # pkg/util/azureclient/azuresdk/armnetwork/generate.go # pkg/util/azureclient/mgmt/network/generate.go # pkg/util/mocks/azureclient/azuresdk/armnetwork/armnetwork.go # pkg/util/mocks/azureclient/mgmt/network/network.go
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
pkg/operator/controllers/previewfeature/nsgflowlogs/nsgflowlogs.go
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| *armnetwork.FlowLogsClient | ||
| } | ||
|
|
||
| func NewFlowLogsClient(subscriptionID string, credential azcore.TokenCredential, options *arm.ClientOptions) (FlowLogsClient, error) { |
There was a problem hiding this comment.
Reference Comment: #3619 (comment)
We should be returning structs, not interfaces as per the go standard. You can refer the roledefinitions client for the usage https://github.com/Azure/ARO-RP/blob/master/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go
For now we can ignore the exisitng clients returning interfaces, but can stop returning the interfaces for new code.
There was a problem hiding this comment.
Thanks! nice catch!
|
/azp run e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Which issue this PR addresses:
Fixes https://issues.redhat.com/browse/ARO-4665
What this PR does / why we need it:
This PR replaces old SDK in preview feature controller.
Test plan for issue:
Test preview feature in local dev cluster according to https://learn.microsoft.com/en-us/azure/openshift/howto-enable-nsg-flowlogs.
Is there any documentation that needs to be updated for this PR?
N/A
How do you know this will function as expected in production?
Enable preview feature.