Remove unused code + refactor after Kubeops removal #5442

castelblanque · 2022-10-07T15:50:49Z

Signed-off-by: Rafa Castelblanque [email protected]

Description of the change

After removing kubeops #5253, there was a lot of old useless backend code left in the Go modules.
This PR removes unused duplicate code related mainly to kubeops and pre-plugins age.
Also refactors some pieces, code under /pkg that is directly related to helm plugin only.

Benefits

Simpler and cleaner code base.

Possible drawbacks

N/A

Applicable issues

fixes Reorganize and clean up code after Kubeops removal #5284

Signed-off-by: Rafa Castelblanque <[email protected]>

netlify · 2022-10-07T15:50:54Z

✅ Deploy Preview for kubeapps-dev canceled.

Name	Link
🔨 Latest commit	`00134ca`
🔍 Latest deploy log	https://app.netlify.com/sites/kubeapps-dev/deploys/634d72e3f575db00089124ff

Signed-off-by: Rafa Castelblanque <[email protected]>

antgamdia

Sorry, I did check this PR but I though I +1ed it 😅
Thanks for the effort in refactoring and moving/deleting the code, really important to keep our codebase tidy up!

beni0888

LGTM! I've added several suggestions, most of them probably not directly related to the scope of this PR, as they likely refer to moved code, and anyway, none of them is blocking. So feel free to ignore them.

beni0888 · 2022-10-17T08:47:47Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_validation.go

+}
+
+// getOCIAppRepositoryTag Gets a tag for the given repo URL & name
+func getOCIAppRepositoryTag(cli httpclient.Client, repoURL string, repoName string) (string, error) {


I think this function has too many responsibilities: it parses/constructs the URL, performs the HTTP request and validates the response, and unmarshal/parses the HTTP response body to get and return the tag. In order to get a shorter function and follow SOLID and clean code principles, I would split it into several auxiliary functions, each with a single responsibility.

beni0888 · 2022-10-17T08:53:29Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_validation.go

+}
+
+// getOCIAppRepositoryMediaType Gets manifests config.MediaType for the given repo URL & Name
+func getOCIAppRepositoryMediaType(cli httpclient.Client, repoURL string, repoName string, tagVersion string) (string, error) {


Here I see more or less the same as in my previous comment. I think that we could have some kind of generic getURL(repoURL, repoName, extraPathSegments... string) to take care of the different URLs generation.

beni0888 · 2022-10-17T09:48:18Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_validation_test.go

+	if err != nil {
+		t.Fatalf("%+v", err)
+	}


I would opt for stretchr/testify functions like require.NotNil for this kind of assertions, as it lead to more readable and maintainable tests.

Agree, it is a cleaner way.
Most part of the code is not new, so we have the traditional way of checking :)

beni0888 · 2022-10-17T09:51:10Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_validation_test.go

+		t.Fatalf("%+v", err)
+	}
+
+	testCases := []struct {


⚠️ Disclaimer: This is a big nitpick. Another possible pattern to clearly separate the name of the tests and their data is to use a map of strings to structs, where the keys would be the names of the tests.

That looks like a good improvement to me, we could start doing it!

beni0888 · 2022-10-17T09:54:36Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_validation_test.go

+			if err != nil {
+				t.Errorf("Unexpected error %v", err)
+			}
+
+			if got, want := fakeClient.request, tc.httpValidator.Req; !cmp.Equal(want, got, cmpOpts...) {
+				t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(want, got, cmpOpts...))
+			}
+
+			if got, want := response, tc.expectedResponse; !cmp.Equal(want, got) {
+				t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(want, got))
+			}


The same here about using testify library assertions.

beni0888 · 2022-10-17T10:39:19Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/utils/chart-client_test.go

+			if ch == nil {
+				t.Errorf("got: nil, want: non-nil")
+			} else if got, want := ch.Name(), "nginx"; got != want {
+				t.Errorf("got: %q, want: %q", got, want)
+			}


Again, I think that testify's assertions would make this block much easier to understand:

require.NotNil(t, ch) require.Equal(t, "nginx", ch.Name())

beni0888 · 2022-10-17T10:41:21Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/utils/chart-client_test.go

+			if got, want := len(requests), expectedLen; got != want {
+				t.Fatalf("got: %d, want %d", got, want)
+			}


The same about using require.Equal()

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/utils/chart-client_test.go

beni0888 · 2022-10-17T10:55:04Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/utils/chart-client_test.go

+	chartURLs []string
+	index     *repo.IndexFile
+	userAgent string
+	// TODO(absoludity): perhaps switch to use httptest instead of our own fake?


I think this would be a good idea 🙂

…lient_test.go Co-authored-by: Jesús Miguel Benito Calzada <[email protected]> Signed-off-by: Rafa Castelblanque <[email protected]>

castelblanque · 2022-10-17T15:22:30Z

@beni0888 Thanks for the thorough review. Some comments I haven't changed because it is actually previously existing code and didn't want to mess with it too much given how big the PR is already.
We can tackle it in future PRs.

Bumps [axios](https://github.com/axios/axios) from 1.2.2 to 1.2.3. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/axios/axios/releases">axios's releases</a>.</em></p> <blockquote> <h2>1.2.3</h2> <h2>Release notes:</h2> <h3>Bug Fixes</h3> <ul> <li><strong>types:</strong> fixed AxiosRequestConfig header interface by refactoring it to RawAxiosRequestConfig; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5420">#5420</a>) (<a href="https://github.com/axios/axios/commit/08119634a22f1d5b19f5c9ea0adccb6d3eebc3bc">0811963</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li><img src="https://avatars.githubusercontent.com/u/12586868?v=4&s=16" alt="avatar" /> <a href="https://github.com/DigitalBrainJS" title="+938/-442 ([#5456](axios/axios#5456) [#5455](axios/axios#5455) [#5453](axios/axios#5453) [#5451](axios/axios#5451) [#5449](axios/axios#5449) [#5447](axios/axios#5447) [#5446](axios/axios#5446) [#5443](axios/axios#5443) [#5442](axios/axios#5442) [#5439](axios/axios#5439) [#5420](axios/axios#5420) )">Dmitriy Mozgovoy</a></li> </ul> </blockquote> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/axios/axios/blob/v1.x/CHANGELOG.md">axios's changelog</a>.</em></p> <blockquote> <h2><a href="https://github.com/axios/axios/compare/1.2.2...1.2.3">1.2.3</a> (2023-01-10)</h2> <h3>Bug Fixes</h3> <ul> <li><strong>types:</strong> fixed AxiosRequestConfig header interface by refactoring it to RawAxiosRequestConfig; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5420">#5420</a>) (<a href="https://github.com/axios/axios/commit/08119634a22f1d5b19f5c9ea0adccb6d3eebc3bc">0811963</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li><img src="https://avatars.githubusercontent.com/u/12586868?v=4&s=16" alt="avatar" /> <a href="https://github.com/DigitalBrainJS" title="+938/-442 ([#5456](axios/axios#5456) [#5455](axios/axios#5455) [#5453](axios/axios#5453) [#5451](axios/axios#5451) [#5449](axios/axios#5449) [#5447](axios/axios#5447) [#5446](axios/axios#5446) [#5443](axios/axios#5443) [#5442](axios/axios#5442) [#5439](axios/axios#5439) [#5420](axios/axios#5420) )">Dmitriy Mozgovoy</a></li> </ul> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/axios/axios/commit/557ed0a7489b1bf62296ea34568eeea8975ff4f9"><code>557ed0a</code></a> chore(ci): fixed publish action; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5470">#5470</a>)</li> <li><a href="https://github.com/axios/axios/commit/484e0b6ed24745df9cadaacc0fbf129114e70d00"><code>484e0b6</code></a> chore(release): v1.2.3 (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5459">#5459</a>)</li> <li><a href="https://github.com/axios/axios/commit/d83316db4a242252db3a2ed7728cb43f8f8f4189"><code>d83316d</code></a> chore(ci): enabled npm publishing; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5460">#5460</a>)</li> <li><a href="https://github.com/axios/axios/commit/d750901deda2994a2d89643e8f18723cfb6b2732"><code>d750901</code></a> chore(ci): added an action to make GitHub & NPM releases when merging a relea...</li> <li><a href="https://github.com/axios/axios/commit/477c71427dc1d03e0f3dced0d65bd7c1b99fd900"><code>477c714</code></a> chore(ci): fixed error in generating changelog with unnecessary spaces; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5455">#5455</a>)</li> <li><a href="https://github.com/axios/axios/commit/e2a1e280f6dfbb4f11ad541dec9541cdbf760ab1"><code>e2a1e28</code></a> chore(ci): improved contributors & PRs sections generator; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5453">#5453</a>)</li> <li><a href="https://github.com/axios/axios/commit/18772ed8fdcd0768a9b520737d81283c04a273f8"><code>18772ed</code></a> chore(ci): improved logging; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5451">#5451</a>)</li> <li><a href="https://github.com/axios/axios/commit/259f5f5aaadfcaf7f3a3fe462d8b0dbbc8004962"><code>259f5f5</code></a> chore(ci): added step of generating a list of contributors for CHANELOG.md; (...</li> <li><a href="https://github.com/axios/axios/commit/d33a3deb82b808f109b598abbf39fd2a1f8da998"><code>d33a3de</code></a> chore(ci): added commit message config; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5447">#5447</a>)</li> <li><a href="https://github.com/axios/axios/commit/ebb9e814436d2f6c7cc65ffecb6ff013539ce961"><code>ebb9e81</code></a> chore(deps): bump json5 from 1.0.1 to 1.0.2 (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5438">#5438</a>)</li> <li>Additional commits viewable in <a href="https://github.com/axios/axios/compare/1.2.2...v1.2.3">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=axios&package-manager=npm_and_yarn&previous-version=1.2.2&new-version=1.2.3)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Remove unused code + refactor

e333eb9

Signed-off-by: Rafa Castelblanque <[email protected]>

vmwclabot added the cla-not-required label Oct 7, 2022

castelblanque marked this pull request as draft October 7, 2022 15:51

Rafa Castelblanque added 4 commits October 7, 2022 17:57

Fixed tests

3854655

Signed-off-by: Rafa Castelblanque <[email protected]>

Relocated some functionality

56360fd

Signed-off-by: Rafa Castelblanque <[email protected]>

Fixed wrong package ref

e15664c

Signed-off-by: Rafa Castelblanque <[email protected]>

Fixed wrong package ref

300321d

Signed-off-by: Rafa Castelblanque <[email protected]>

castelblanque changed the title ~~Remove unused code + refactor~~ Remove unused code + refactor after Kubeops removal Oct 7, 2022

castelblanque marked this pull request as ready for review October 10, 2022 07:15

castelblanque requested review from antgamdia and beni0888 October 10, 2022 10:53

antgamdia approved these changes Oct 17, 2022

View reviewed changes

beni0888 approved these changes Oct 17, 2022

View reviewed changes

castelblanque and others added 2 commits October 17, 2022 17:20

Update cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/utils/chart-c…

fa14cfe

…lient_test.go Co-authored-by: Jesús Miguel Benito Calzada <[email protected]> Signed-off-by: Rafa Castelblanque <[email protected]>

Update cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/utils/chart-c…

00134ca

…lient_test.go Co-authored-by: Jesús Miguel Benito Calzada <[email protected]> Signed-off-by: Rafa Castelblanque <[email protected]>

castelblanque merged commit 50c467a into main Oct 18, 2022

castelblanque deleted the 5284-remove-refactor-old-code branch October 18, 2022 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unused code + refactor after Kubeops removal #5442

Remove unused code + refactor after Kubeops removal #5442

castelblanque commented Oct 7, 2022

netlify bot commented Oct 7, 2022 •

edited

Loading

antgamdia left a comment

beni0888 left a comment

beni0888 Oct 17, 2022

beni0888 Oct 17, 2022

beni0888 Oct 17, 2022

castelblanque Oct 17, 2022

beni0888 Oct 17, 2022

castelblanque Oct 17, 2022

beni0888 Oct 17, 2022

beni0888 Oct 17, 2022

beni0888 Oct 17, 2022

beni0888 Oct 17, 2022

castelblanque commented Oct 17, 2022

Remove unused code + refactor after Kubeops removal #5442

Remove unused code + refactor after Kubeops removal #5442

Conversation

castelblanque commented Oct 7, 2022

Description of the change

Benefits

Possible drawbacks

Applicable issues

netlify bot commented Oct 7, 2022 • edited Loading

✅ Deploy Preview for kubeapps-dev canceled.

antgamdia left a comment

Choose a reason for hiding this comment

beni0888 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

castelblanque commented Oct 17, 2022

netlify bot commented Oct 7, 2022 •

edited

Loading