reduce complexity of ManagedCluster spec #4635

bravebeaver · 2024-03-11T09:13:05Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

to reduce the complexity of the Params method which exceeds golangci-lint limits.

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 #3810

Special notes for your reviewer:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

NONE

k8s-ci-robot · 2024-03-11T09:13:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jont828 for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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 · 2024-03-11T09:13:15Z

Hi @bravebeaver. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

bravebeaver · 2024-03-11T09:18:24Z

try to follow the pattern to build parts of managedCluster. some properties are left inside the main Params() method if they are

trivial (e.g., spec.name)
too complicated (e.g., agentpool profile)

func (s *spec) buildxxx(*managedCluster)

mboersma · 2024-03-11T14:48:26Z

/ok-to-test

mboersma · 2024-03-13T14:20:59Z

Thanks @bravebeaver, this turned out to be a lot of work! I think your approach to refactoring is reasonable.

Apparently the TestParameters/no_existing_managed_cluster unit test is still failing though. Please take a look whenever time permits.

bravebeaver · 2024-03-19T00:47:03Z

@mboersma will do. it seems to be a trait of refactoring that apparently nothing is changed but a lot has been changed.. 😅.

Bumps [softprops/action-gh-release](https://github.com/softprops/action-gh-release) from 1 to 2. - [Release notes](https://github.com/softprops/action-gh-release/releases) - [Changelog](https://github.com/softprops/action-gh-release/blob/master/CHANGELOG.md) - [Commits](softprops/action-gh-release@de2c0eb...d99959e) --- updated-dependencies: - dependency-name: softprops/action-gh-release dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.20.0 to 0.21.0. - [Commits](golang/crypto@v0.20.0...v0.21.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [golang.org/x/mod](https://github.com/golang/mod) from 0.15.0 to 0.16.0. - [Commits](golang/mod@v0.15.0...v0.16.0) --- updated-dependencies: - dependency-name: golang.org/x/mod dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [tj-actions/changed-files](https://github.com/tj-actions/changed-files) from 42.0.5 to 42.1.0. - [Release notes](https://github.com/tj-actions/changed-files/releases) - [Changelog](https://github.com/tj-actions/changed-files/blob/main/HISTORY.md) - [Commits](tj-actions/changed-files@800a282...aa08304) --- updated-dependencies: - dependency-name: tj-actions/changed-files dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo) from 2.15.0 to 2.16.0. - [Release notes](https://github.com/onsi/ginkgo/releases) - [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md) - [Commits](onsi/ginkgo@v2.15.0...v2.16.0) --- updated-dependencies: - dependency-name: github.com/onsi/ginkgo/v2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: Jack Francis <[email protected]>

* Change logic for only one file Signed-off-by: David Tesar <[email protected]> * Filter proper changed files Signed-off-by: David Tesar <[email protected]> * Fix logic Signed-off-by: David Tesar <[email protected]> --------- Signed-off-by: David Tesar <[email protected]>

Bumps [tj-actions/changed-files](https://github.com/tj-actions/changed-files) from 42.1.0 to 43.0.0. - [Release notes](https://github.com/tj-actions/changed-files/releases) - [Changelog](https://github.com/tj-actions/changed-files/blob/main/HISTORY.md) - [Commits](tj-actions/changed-files@aa08304...77af4be) --- updated-dependencies: - dependency-name: tj-actions/changed-files dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [softprops/action-gh-release](https://github.com/softprops/action-gh-release) from 2.0.2 to 2.0.4. - [Release notes](https://github.com/softprops/action-gh-release/releases) - [Changelog](https://github.com/softprops/action-gh-release/blob/master/CHANGELOG.md) - [Commits](softprops/action-gh-release@d99959e...9d7c94c) --- updated-dependencies: - dependency-name: softprops/action-gh-release dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [github/codeql-action](https://github.com/github/codeql-action) from 3.24.6 to 3.24.8. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@8a470fd...05963f4) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [actions/checkout](https://github.com/actions/checkout) from 4.1.1 to 4.1.2. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@b4ffde6...9bb5618) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: David Tesar <[email protected]>

bravebeaver · 2024-03-19T01:27:33Z

/retest

mboersma

/lgtm

Thanks for doing this and my apologies for not reviewing sooner!

Could you squash the commits and rebase against HEAD?

mboersma · 2024-03-28T16:50:32Z

azure/services/managedclusters/spec.go

- }
- prev := &asocontainerservicev1preview.ManagedCluster{}
- if err := prev.ConvertFrom(hub); err != nil {
+ preview, err := convertV1ToPreview(managedCluster)


Thank you for changing this: prev is an ambiguous variable name. 👍🏻

k8s-ci-robot · 2024-03-28T16:53:02Z

LGTM label has been added.

Git tree hash: 7f7158d445759f15919bf668481cf217b954a02e

codecov · 2024-03-28T17:01:47Z

Codecov Report

Attention: Patch coverage is 82.95964% with 38 lines in your changes are missing coverage. Please review.

Project coverage is 62.79%. Comparing base (ad6d0d9) to head (f084ef2).
Report is 74 commits behind head on main.

Files	Patch %	Lines
azure/services/managedclusters/spec.go	82.72%	26 Missing and 12 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4635      +/-   ##
==========================================
+ Coverage   62.73%   62.79%   +0.06%     
==========================================
  Files         192      192              
  Lines       15641    15691      +50     
==========================================
+ Hits         9812     9853      +41     
- Misses       5146     5152       +6     
- Partials      683      686       +3

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

k8s-ci-robot · 2024-04-13T06:51:36Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

bravebeaver added 6 commits March 10, 2024 21:57

construct network profile separately

120659a

construct httpProxyConfig separately

2969004

fix golang usage according to linter

882dfd1

handle preview separately

904d1e3

separate out conversion between preview and v1

8351ac3

name builder functions consistently

42a8004

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. labels Mar 11, 2024

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 11, 2024

k8s-ci-robot requested review from mboersma and nojnhuh March 11, 2024 09:13

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 11, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 11, 2024

dependabot bot and others added 9 commits March 19, 2024 14:06

Enable E2E testing for v1.15 release cycle

07e1270

Signed-off-by: Jack Francis <[email protected]>

Bump CAPI to v1.6.3

72f46a2

Bump starting CAPI to v1.5.7 for clusterctl upgrade test

58c2c28

dependabot bot and others added 7 commits March 19, 2024 14:06

First pass at process enhancements

bdd431a

Signed-off-by: David Tesar <[email protected]>

get version from existing

8e69b33

Merge branch 'kubernetes-sigs:main' into beileih/3810ReduceComplexity

f084ef2

dtzar assigned mboersma Mar 28, 2024

mboersma reviewed Mar 28, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 28, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reduce complexity of ManagedCluster spec #4635

reduce complexity of ManagedCluster spec #4635

bravebeaver commented Mar 11, 2024

k8s-ci-robot commented Mar 11, 2024

k8s-ci-robot commented Mar 11, 2024

bravebeaver commented Mar 11, 2024 •

edited

mboersma commented Mar 11, 2024

mboersma commented Mar 13, 2024

bravebeaver commented Mar 19, 2024

bravebeaver commented Mar 19, 2024

mboersma left a comment

mboersma Mar 28, 2024

k8s-ci-robot commented Mar 28, 2024

codecov bot commented Mar 28, 2024 •

edited

k8s-ci-robot commented Apr 13, 2024

reduce complexity of ManagedCluster spec #4635

Are you sure you want to change the base?

reduce complexity of ManagedCluster spec #4635

Conversation

bravebeaver commented Mar 11, 2024

k8s-ci-robot commented Mar 11, 2024

k8s-ci-robot commented Mar 11, 2024

bravebeaver commented Mar 11, 2024 • edited

mboersma commented Mar 11, 2024

mboersma commented Mar 13, 2024

bravebeaver commented Mar 19, 2024

bravebeaver commented Mar 19, 2024

mboersma left a comment

Choose a reason for hiding this comment

mboersma Mar 28, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 28, 2024

codecov bot commented Mar 28, 2024 • edited

Codecov Report

k8s-ci-robot commented Apr 13, 2024

bravebeaver commented Mar 11, 2024 •

edited

codecov bot commented Mar 28, 2024 •

edited