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

Configurable global ns for Helm in the plugin's config #5339

Merged
merged 8 commits into from
Sep 13, 2022
Merged

Configurable global ns for Helm in the plugin's config #5339

merged 8 commits into from
Sep 13, 2022

Conversation

antgamdia
Copy link
Contributor

Description of the change

This PR mainly adds a way to set the global namespace via the Helm plugin while ensuring we are not breaking backward compatibility. Besides, it performs plenty of renames from globalRepo to something more helm-specific.

Benefits

The way to set global namespaces is more consistent across plugins.

Possible drawbacks

There are two (kinda misleading) ways to set the global namespace: 1) relying on the current "kubeapps's namespace + suffix"; 2) manually set via the Helm plugin config.

Applicable issues

Additional information

N/A

@netlify
Copy link

netlify bot commented Sep 12, 2022 •
edited
Loading

✅ Deploy Preview for kubeapps-dev canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 07d7f8c
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/63203f0f91f8f200091fec02

antgamdia
antgamdia commented Sep 12, 2022
View reviewed changes
chart/kubeapps/templates/_helpers.tpl
*/}}
{{- define "kubeapps.globalReposNamespace" -}}
{{- printf "%s%s" .Release.Namespace .Values.apprepository.globalReposNamespaceSuffix -}}
{{- define "kubeapps.helmGlobalPackagingNamespace" -}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were already using this helper function to retrieve the namespace, so I've changed it to: "use the globalPackagingNamespace in the plugin or fall back to the previous behavior"

chart/kubeapps/templates/kubeappsapis/deployment.yaml
@@ -108,7 +108,7 @@ spec:
- --pinniped-proxy-ca-cert=/etc/pinniped-proxy-tls/ca.crt
{{- end }}
{{- end }}
- --global-repos-namespace={{ include "kubeapps.globalReposNamespace" . }}
- --global-repos-namespace={{ include "kubeapps.helmGlobalPackagingNamespace" . }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't changed the --global-repos-namespace argument since cobra CLI would panic if passing an unrecognized flag (and this is something that can happen when upgrading from a previous chart version).

I'd like to get rid of this command line "global repo" instead, but not right now for ensuring a smooth transition.

chart/kubeapps/values.yaml
v1alpha1:
## @param kubeappsapis.pluginConfig.helm.packages.v1alpha1.globalPackagingNamespace Custom global packaging namespace. Using this value will override the current "kubeapps release namespace + suffix" pattern and will create a new namespace if not exists.
globalPackagingNamespace: ""
## @param kubeappsapis.pluginConfig.helm.packages.v1alpha1.userManagedSecrets Default policy for handling repository secrets, either managed by the user or by kubeapps-apis
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've squeezed this userManagedSecrets as it was a hardcoded false value 😅

castelblanque
castelblanque reviewed Sep 13, 2022
View reviewed changes
chart/kubeapps/templates/_helpers.tpl
Comment on lines +246 to +250
{{- if .Values.kubeappsapis.pluginConfig.helm.packages.v1alpha1.globalPackagingNamespace }}
{{- printf "%s" .Values.kubeappsapis.pluginConfig.helm.packages.v1alpha1.globalPackagingNamespace -}}
{{- else -}}
{{- printf "%s%s" .Release.Namespace .Values.apprepository.globalReposNamespaceSuffix -}}
{{- end -}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perfect! We give now priority to the right plugin-scoped value. In the future we will deprecate the apprepository.globalReposNamespaceSuffix.

castelblanque
castelblanque approved these changes Sep 13, 2022
View reviewed changes
Copy link
Collaborator

@castelblanque castelblanque left a comment

Choose a reason for hiding this comment

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

LGTM

@antgamdia antgamdia merged commit baff9b6 into vmware-tanzu:main Sep 13, 2022
@antgamdia antgamdia deleted the 5338-configurableGlobalNsHelm branch September 13, 2022 10:09
absoludity added a commit that referenced this pull request Feb 24, 2023
@absoludity
Ensure loaded plugin config is saved to helm server. (#6022)
56bc140 
<!--
Before you open the request please review the following guidelines and
tips to help it be more easily integrated:

 - Describe the scope of your change - i.e. what the change does.
 - Describe any known limitations with your change.
- Please run any tests or examples that can exercise your modified code.

 Thank you for contributing!
 -->

### Description of the change

<!-- Describe the scope of your change - i.e. what the change does. -->
In [#5339 we inadvertently
switched](https://github.com/vmware-tanzu/kubeapps/pull/5339/files#diff-da1b1406426f28c45f4b63a5ae7547e9c29747e118bf70df6a4694d575710f94R104)
from an [assignment](https://go.dev/ref/spec#Assignment_statements) to a
[short variable
declaration](https://go.dev/ref/spec#Short_variable_declarations):

```diff
-		pluginConfig, err = common.ParsePluginConfig(pluginConfigPath)
+		pluginConfig, err := common.ParsePluginConfig(pluginConfigPath)
```

which meant that, in context, a *new* `pluginConfig` var was created in
that scope, so that the actual `pluginConfig` var from the outside scope
remained empty.

### Benefits

Plugin config works again for the Helm plugin.

### Possible drawbacks

<!-- Describe any known limitations with your change -->

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- fixes #6020 

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->
I spent some time trying to write a test for this, but I could either
extract the 5 lines to a separate function and test that or test the
`NewServer` function itself. The latter is very difficult because it
instantiates a bunch of network requests, and the former seems redundant
since we already extract the functionality to the
`common.ParsePluginConfig()` function... it's really just a golang
gotcha that is important to be aware of: `a := foo` is equivalent to
`var a atype \n a = foo` and so is always creating a new variable in
that scope.

Signed-off-by: Michael Nelson <[email protected]>
absoludity pushed a commit that referenced this pull request Apr 28, 2023
@dependabot
Bump axios from 1.3.6 to 1.4.0 in /integration (#6235)
2e0ad79 
Bumps [axios](https://github.com/axios/axios) from 1.3.6 to 1.4.0.
<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>Release v1.4.0</h2>
<h2>Release notes:</h2>
<h3>Bug Fixes</h3>
<ul>
<li><strong>formdata:</strong> add <code>multipart/form-data</code>
content type for FormData payload on custom client environments; (<a
href="https://redirect.github.com/axios/axios/issues/5678">#5678</a>)
(<a
href="https://github.com/axios/axios/commit/bbb61e70cb1185adfb1cbbb86eaf6652c48d89d1">bbb61e7</a>)</li>
<li><strong>package:</strong> export package internals with unsafe path
prefix; (<a
href="https://redirect.github.com/axios/axios/issues/5677">#5677</a>)
(<a
href="https://github.com/axios/axios/commit/df38c949f26414d88ba29ec1e353c4d4f97eaf09">df38c94</a>)</li>
</ul>
<h3>Features</h3>
<ul>
<li><strong>dns:</strong> added support for a custom lookup function;
(<a
href="https://redirect.github.com/axios/axios/issues/5339">#5339</a>)
(<a
href="https://github.com/axios/axios/commit/2701911260a1faa5cc5e1afe437121b330a3b7bb">2701911</a>)</li>
<li><strong>types:</strong> export <code>AxiosHeaderValue</code> type.
(<a
href="https://redirect.github.com/axios/axios/issues/5525">#5525</a>)
(<a
href="https://github.com/axios/axios/commit/726f1c8e00cffa0461a8813a9bdcb8f8b9d762cf">726f1c8</a>)</li>
</ul>
<h3>Performance Improvements</h3>
<ul>
<li><strong>merge-config:</strong> optimize mergeConfig performance by
avoiding duplicate key visits; (<a
href="https://redirect.github.com/axios/axios/issues/5679">#5679</a>)
(<a
href="https://github.com/axios/axios/commit/e6f7053bf1a3e87cf1f9da8677e12e3fe829d68e">e6f7053</a>)</li>
</ul>
<h3>Contributors to this release</h3>
<ul>
<li><!-- raw HTML omitted --> <a
href="https://github.com/DigitalBrainJS" title="+151/-16
([#5684](axios/axios#5684)
[#5339](axios/axios#5339)
[#5679](axios/axios#5679)
[#5678](axios/axios#5678)
[#5677](axios/axios#5677) )">Dmitriy
Mozgovoy</a></li>
<li><!-- raw HTML omitted --> <a
href="https://github.com/arthurfiorette" title="+19/-19
([#5525](axios/axios#5525) )">Arthur
Fiorette</a></li>
<li><!-- raw HTML omitted --> <a href="https://github.com/npiyush97"
title="+2/-18 ([#5670](axios/axios#5670)
)">PIYUSH NEGI</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>
<h1><a
href="https://github.com/axios/axios/compare/v1.3.6...v1.4.0">1.4.0</a>
(2023-04-27)</h1>
<h3>Bug Fixes</h3>
<ul>
<li><strong>formdata:</strong> add <code>multipart/form-data</code>
content type for FormData payload on custom client environments; (<a
href="https://redirect.github.com/axios/axios/issues/5678">#5678</a>)
(<a
href="https://github.com/axios/axios/commit/bbb61e70cb1185adfb1cbbb86eaf6652c48d89d1">bbb61e7</a>)</li>
<li><strong>package:</strong> export package internals with unsafe path
prefix; (<a
href="https://redirect.github.com/axios/axios/issues/5677">#5677</a>)
(<a
href="https://github.com/axios/axios/commit/df38c949f26414d88ba29ec1e353c4d4f97eaf09">df38c94</a>)</li>
</ul>
<h3>Features</h3>
<ul>
<li><strong>dns:</strong> added support for a custom lookup function;
(<a
href="https://redirect.github.com/axios/axios/issues/5339">#5339</a>)
(<a
href="https://github.com/axios/axios/commit/2701911260a1faa5cc5e1afe437121b330a3b7bb">2701911</a>)</li>
<li><strong>types:</strong> export <code>AxiosHeaderValue</code> type.
(<a
href="https://redirect.github.com/axios/axios/issues/5525">#5525</a>)
(<a
href="https://github.com/axios/axios/commit/726f1c8e00cffa0461a8813a9bdcb8f8b9d762cf">726f1c8</a>)</li>
</ul>
<h3>Performance Improvements</h3>
<ul>
<li><strong>merge-config:</strong> optimize mergeConfig performance by
avoiding duplicate key visits; (<a
href="https://redirect.github.com/axios/axios/issues/5679">#5679</a>)
(<a
href="https://github.com/axios/axios/commit/e6f7053bf1a3e87cf1f9da8677e12e3fe829d68e">e6f7053</a>)</li>
</ul>
<h3>Contributors to this release</h3>
<ul>
<li><!-- raw HTML omitted --> <a
href="https://github.com/DigitalBrainJS" title="+151/-16
([#5684](axios/axios#5684)
[#5339](axios/axios#5339)
[#5679](axios/axios#5679)
[#5678](axios/axios#5678)
[#5677](axios/axios#5677) )">Dmitriy
Mozgovoy</a></li>
<li><!-- raw HTML omitted --> <a
href="https://github.com/arthurfiorette" title="+19/-19
([#5525](axios/axios#5525) )">Arthur
Fiorette</a></li>
<li><!-- raw HTML omitted --> <a href="https://github.com/npiyush97"
title="+2/-18 ([#5670](axios/axios#5670)
)">PIYUSH NEGI</a></li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/axios/axios/commit/21a5ad34c4a5956d81d338059ac0dd34a19ed094"><code>21a5ad3</code></a>
chore(release): v1.4.0 (<a
href="https://redirect.github.com/axios/axios/issues/5683">#5683</a>)</li>
<li><a
href="https://github.com/axios/axios/commit/d627610d0c427de57c10618b36fa97814e2a75f0"><code>d627610</code></a>
chore(utils): refactored isAsyncFn util to avoid inlining additional
Babel he...</li>
<li><a
href="https://github.com/axios/axios/commit/e18fdd893dfc67630c33fb6744d1b99d72857d92"><code>e18fdd8</code></a>
refactor: remove deprecated url-search-params polyfill for
URLSearchParams (#...</li>
<li><a
href="https://github.com/axios/axios/commit/726f1c8e00cffa0461a8813a9bdcb8f8b9d762cf"><code>726f1c8</code></a>
feat(types): export <code>AxiosHeaderValue</code> type. (<a
href="https://redirect.github.com/axios/axios/issues/5525">#5525</a>)</li>
<li><a
href="https://github.com/axios/axios/commit/2701911260a1faa5cc5e1afe437121b330a3b7bb"><code>2701911</code></a>
feat(dns): added support for a custom lookup function; (<a
href="https://redirect.github.com/axios/axios/issues/5339">#5339</a>)</li>
<li><a
href="https://github.com/axios/axios/commit/e6f7053bf1a3e87cf1f9da8677e12e3fe829d68e"><code>e6f7053</code></a>
perf(merge-config): optimize mergeConfig performance by avoiding
duplicate ke...</li>
<li><a
href="https://github.com/axios/axios/commit/bbb61e70cb1185adfb1cbbb86eaf6652c48d89d1"><code>bbb61e7</code></a>
fix(formdata): add <code>multipart/form-data</code> content type for
FormData payload on...</li>
<li><a
href="https://github.com/axios/axios/commit/df38c949f26414d88ba29ec1e353c4d4f97eaf09"><code>df38c94</code></a>
fix(package): export package internals with unsafe path prefix; (<a
href="https://redirect.github.com/axios/axios/issues/5677">#5677</a>)</li>
<li>See full diff in <a
href="https://github.com/axios/axios/compare/v1.3.6...v1.4.0">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.3.6&new-version=1.4.0)](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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@castelblanque castelblanque castelblanque approved these changes

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move globalReposNamespace flag to be Helm plugin-specific
3 participants
@antgamdia @castelblanque @vmwclabot