Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SDK2: Remove old SDK from Dynamic/ValidateQuota #3889

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

SDK2: Remove old SDK from Dynamic/ValidateQuota #3889

wants to merge 5 commits into from

Conversation

bitoku
Copy link
Collaborator

@bitoku bitoku commented Oct 8, 2024

Which issue this PR addresses:

Fixes https://issues.redhat.com/browse/ARO-4665

What this PR does / why we need it:

This PR replaces the old SDK to the new SDK2.

It affects Dynamic pkg (used in validateResources step) and ValidateQuota.go (used in openshiftcluster_putorpatch.go).

Test plan for issue:

e2e, ci
e2e creates a cluster, which verifies the validateResource step and ValidateQuota.

Is there any documentation that needs to be updated for this PR?

na

How do you know this will function as expected in production?

Cluster creation will verify the change

@bitoku
Copy link
Collaborator Author

bitoku commented Oct 8, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

kimorris27
kimorris27 previously approved these changes Oct 8, 2024
View reviewed changes
jaitaiwan
jaitaiwan requested changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@jaitaiwan jaitaiwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM. I think there's some convention questions as well as a question around why the use of v2 vs v6 (the current version in the repo)

pkg/frontend/quota_test.go Show resolved Hide resolved
pkg/util/azureclient/azuresdk/armnetwork/usage.go Show resolved Hide resolved
pkg/validate/dynamic/dynamic.go Show resolved Hide resolved
@bitoku bitoku requested a review from jaitaiwan October 16, 2024 08:38
jaitaiwan
jaitaiwan requested changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@jaitaiwan jaitaiwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to hawkowl we realised there are fakes etc available so we no longer have need for generating our own mocks. See pkg/util/liveconfig/hive_test.go in my PR https://github.com/Azure/ARO-RP/pull/3889/files

@bitoku
Copy link
Collaborator Author

bitoku commented Oct 25, 2024

it's better to keep the code as it is because the PR aims to replace the usage of mgmt clients, and changes of the way of testing (or change the way of listing) is out of scope.
Also this is just a migration, so it's dangerous to change the test code at the same time though it's not necessary.
We should keep the test code the same as much as possible otherwise we can't ensure that the behaviour hasn't been changed.

jaitaiwan
jaitaiwan previously approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@jaitaiwan jaitaiwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; 1 question

pkg/validate/dynamic/loadbalancerprofile_test.go Outdated
"github.com/Azure/go-autorest/autorest/to"
"github.com/sirupsen/logrus"
"go.uber.org/mock/gomock"
"k8s.io/utils/ptr"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we using this lib instead of just going with to.Ptr?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's already deprecated.
https://github.com/Azure/go-autorest

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The to functionality moved into the core sdk: github.com/Azure/azure-sdk-for-go/sdk/azcore/to

Copy link
Collaborator Author

@bitoku bitoku Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are already usages of ptr.To in other places. We should discuss which should be used, but anyway we need to change the code from to.*Ptr to to.Ptr or ptr.To.
I don't have any preference.

Copy link
Collaborator Author

@bitoku bitoku Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started a discussion
https://redhat-internal.slack.com/archives/C02ULBRS68M/p1731940533915429

@github-actions github-actions bot added the needs-rebase branch needs a rebase label Nov 4, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 4, 2024

Please rebase pull request.

@bitoku bitoku dismissed stale reviews from jaitaiwan and kimorris27 via 4b757f8 November 5, 2024 13:20
@github-actions github-actions bot removed the needs-rebase branch needs a rebase label Nov 5, 2024
@bitoku
Copy link
Collaborator Author

bitoku commented Nov 5, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@github-actions github-actions bot added the needs-rebase branch needs a rebase label Nov 7, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2024

Please rebase pull request.

@bitoku
Copy link
Collaborator Author

bitoku commented Nov 20, 2024

This will conflict with #3922.
After it's merged, I'll update this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkeohane dkeohane dkeohane left review comments

@jaitaiwan jaitaiwan jaitaiwan left review comments

@kimorris27 kimorris27 kimorris27 approved these changes

@jewzaam jewzaam Awaiting requested review from jewzaam jewzaam is a code owner

@bennerv bennerv Awaiting requested review from bennerv bennerv is a code owner

@hawkowl hawkowl Awaiting requested review from hawkowl hawkowl is a code owner

@rogbas rogbas Awaiting requested review from rogbas rogbas is a code owner

@petrkotas petrkotas Awaiting requested review from petrkotas petrkotas is a code owner

@jharrington22 jharrington22 Awaiting requested review from jharrington22 jharrington22 is a code owner

@cblecker cblecker Awaiting requested review from cblecker cblecker is a code owner

@cadenmarchese cadenmarchese Awaiting requested review from cadenmarchese cadenmarchese is a code owner

@UlrichSchlueter UlrichSchlueter Awaiting requested review from UlrichSchlueter UlrichSchlueter is a code owner

@SudoBrendan SudoBrendan Awaiting requested review from SudoBrendan SudoBrendan is a code owner

@yjst2012 yjst2012 Awaiting requested review from yjst2012 yjst2012 is a code owner

@anshulvermapatel anshulvermapatel Awaiting requested review from anshulvermapatel anshulvermapatel is a code owner

@hlipsig hlipsig Awaiting requested review from hlipsig hlipsig is a code owner

@tiguelu tiguelu Awaiting requested review from tiguelu tiguelu is a code owner

@SrinivasAtmakuri SrinivasAtmakuri Awaiting requested review from SrinivasAtmakuri SrinivasAtmakuri is a code owner

@mociarain mociarain Awaiting requested review from mociarain mociarain is a code owner

@tsatam tsatam Awaiting requested review from tsatam tsatam is a code owner

@fahlmant fahlmant Awaiting requested review from fahlmant fahlmant is a code owner

Assignees
No one assigned
Labels
needs-rebase branch needs a rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bitoku @jaitaiwan @dkeohane @kimorris27