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

Update privateclusters e2e test for SDKv2 #4268

Merged
merged 2 commits into from
Dec 2, 2023
Merged

Update privateclusters e2e test for SDKv2 #4268

merged 2 commits into from
Dec 2, 2023

Conversation

mboersma
Copy link
Contributor

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Removes the last vestige of Azure SDK v1 code from CAPZ, specifically the privatecluster e2e test.

Which issue(s) this PR fixes:

Fixes #4056

Special notes for your reviewer:

  • cherry-pick candidate

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 14, 2023
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 14, 2023
@mboersma
Copy link
Contributor Author

/retitle [WIP] Update privateclusters e2e test for SDKv2

@k8s-ci-robot k8s-ci-robot changed the title Update privateclusters e2e test for SDKv2 [WIP] Update privateclusters e2e test for SDKv2 Nov 14, 2023
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 14, 2023
@mboersma
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-optional

@codecov Codecov
Copy link

codecov bot commented Nov 14, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8c99db0) 60.03% compared to head (9111d0a) 60.03%.
Report is 16 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #4268   +/-   ##
=======================================
  Coverage   60.03%   60.03%           
=======================================
  Files         192      192           
  Lines       19297    19297           
=======================================
  Hits        11585    11585           
  Misses       7075     7075           
  Partials      637      637

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mboersma
Copy link
Contributor Author

mboersma commented Nov 14, 2023 •
edited
Loading

All the -optional test specs failed in the same way, apparently unrelated to this change. I'll run them once more.

/test pull-cluster-api-provider-azure-e2e-optional

@mboersma mboersma force-pushed the privateclusters-e2e-sdk-v2 branch from f2669b1 to 0578410 Compare November 14, 2023 19:12
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 14, 2023
@mboersma
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-optional

@mboersma mboersma marked this pull request as draft November 14, 2023 19:15
@mboersma
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-optional

1 similar comment
@mboersma
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-optional

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2023
@mboersma mboersma force-pushed the privateclusters-e2e-sdk-v2 branch from 0578410 to f5d251f Compare November 22, 2023 21:05
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2023
@mboersma
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-optional

@mboersma mboersma force-pushed the privateclusters-e2e-sdk-v2 branch from f5d251f to c1bdd30 Compare November 27, 2023 15:58
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 27, 2023
@mboersma
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-optional

@mboersma mboersma force-pushed the privateclusters-e2e-sdk-v2 branch from c1bdd30 to f649f6a Compare November 27, 2023 18:54
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 27, 2023
@mboersma
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-optional

1 similar comment
@mboersma
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-optional

@mboersma mboersma added this to the v1.13 milestone Nov 29, 2023
@mboersma
Copy link
Contributor Author

w00t it passed (ignore the Flatcar failure). I'll squash and promote from draft.

@mboersma mboersma force-pushed the privateclusters-e2e-sdk-v2 branch from 44d780a to 9111d0a Compare November 29, 2023 17:53
@mboersma mboersma marked this pull request as ready for review November 29, 2023 17:53
@k8s-ci-robot k8s-ci-robot requested a review from Jont828 November 29, 2023 17:53
@mboersma mboersma changed the title [WIP] Update privateclusters e2e test for SDKv2 Update privateclusters e2e test for SDKv2 Nov 29, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2023
@mboersma
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-optional

@mboersma
Copy link
Contributor Author

Lots of red on that run, but it did pass the privateclusters e2e spec: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/4268/pull-cluster-api-provider-azure-e2e-optional/1729921750407319552

@nojnhuh
Copy link
Contributor

nojnhuh commented Nov 29, 2023

@mboersma For context, what was the heart of the reason that this test was more complicated to update to SDK v2 than other tests?

@mboersma
Copy link
Contributor Author

@nojnhuh the main differences were:

  • multiple clients from different services involved
  • some generic resource code that can't be replicated in SDKv2 (which I removed)

But overall, the issue was that I previously updated this all at once, in a flurry of test and service refactoring, and it failed in a way I wasn't able to diagnose.

So I tried again, replacing the code bit by bit, and this time it succeeded. It took quite a while because I'm not able to run this particular test locally, so I had to make it a draft PR and then manually run the -optional test (and ignore the Flatcar failure).

@mboersma
Copy link
Contributor Author

/retest-required

nojnhuh
nojnhuh reviewed Nov 29, 2023
View reviewed changes
Copy link
Contributor

@nojnhuh nojnhuh left a comment

Choose a reason for hiding this comment

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

/lgtm

.golangci.yml
@@ -140,10 +140,6 @@ issues:
- prealloc
- path: ^test/
text: exported (.+) should have comment( \(or a comment on this block\))? or be unexported
# Need to use the deprecated module "github.com/Azure/azure-sdk-for-go/services" till issue #2670 is addressed.
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

🥳

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 29, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b1b52eb0ee593bd2d31b4466a94114a4bdb82ac3

@mboersma
Copy link
Contributor Author

/retest

@mboersma
Copy link
Contributor Author

The -optional test failed again, but the privateclusters spec succeeded.

@mboersma
Copy link
Contributor Author

mboersma commented Dec 1, 2023

Everything passed except for the Flatcar spec, so I think this is ready for review.

@CecileRobertMichon
Copy link
Contributor

Flatcar should be fixed now

/retest

CecileRobertMichon
CecileRobertMichon reviewed Dec 1, 2023
View reviewed changes
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2023
@k8s-ci-robot k8s-ci-robot merged commit e2ef58e into kubernetes-sigs:main Dec 2, 2023
29 checks passed
@mboersma mboersma deleted the privateclusters-e2e-sdk-v2 branch December 4, 2023 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nojnhuh nojnhuh nojnhuh left review comments

@CecileRobertMichon CecileRobertMichon CecileRobertMichon left review comments

@Jont828 Jont828 Awaiting requested review from Jont828

Assignees

@CecileRobertMichon CecileRobertMichon

@nojnhuh nojnhuh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
CAPZ Planning
Archived in project
Milestone
v1.13
Development

Successfully merging this pull request may close these issues.

Convert private clusters e2e tests to SDKv2
4 participants
@mboersma @nojnhuh @k8s-ci-robot @CecileRobertMichon