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.

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

Add helm template apiVersions flag #894

Closed

Conversation

RichardWLaub
Copy link
Contributor

Proposed changes

Allows me to pass the --api-versions flag to helm template so I can deploy helm chart resources with the following conditional:

{{- if and ( .Capabilities.APIVersions.Has "monitoring.coreos.com/v1" ) ( .Values.serviceMonitor.enabled ) }}

Note that example was taken from the prometheus-cloudwatch-exporter helm chart.

Related issues

Fixes #196

@RichardWLaub
Copy link
Contributor Author

@hausdorff what do you think of this approach to solve the .Capabilities issue?

@lukehoban
Copy link
Contributor

One quick note - is it possible to add a test? There are some tests in /tests/examples/helm/ and /tests/integration/istio/ for example - would be nice to add a test that covers this.

@RichardWLaub
Copy link
Contributor Author

@lukehoban thanks for pointing out the example tests. Working on adding one now

Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

@RichardWLaub Thanks for the PR, and sorry for the delayed response! Just a few more suggestions, and then I'll run this through CI and get it merged.

/**
* The optional kubernetes api versions used for Capabilities.APIVersions.
*/
apiVersions?: pulumi.Input<string[]>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
apiVersions?: pulumi.Input<string[]>;
apiVersions?: pulumi.Input<pulumi.Input<string>[]>;

new k8s.helm.v2.Chart("api-versions", {
apiVersions: ["foo", "bar"],
path: "helm-api-versions",
transformations: [
Copy link
Member

Choose a reason for hiding this comment

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

With the suggested update to the Pod manifest, you can update this to

new k8s.helm.v2.Chart("api-versions", {
  apiVersions: ["foo", "bar"],
  path: "helm-api-versions",
  namespace: namespace.metadata.name,
});

import * as k8s from "@pulumi/kubernetes";

const namespace = new k8s.core.v1.Namespace("test");
const namespaceName = namespace.metadata.name;
Copy link
Member

Choose a reason for hiding this comment

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

Not needed with suggested update.

Suggested change
const namespaceName = namespace.metadata.name;

@RichardWLaub RichardWLaub force-pushed the rl/helm-template-api-versions branch from 512fdf8 to ba8ce52 Compare January 8, 2020 22:51
Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

One nit on the code, but overall LGTM

Could you also update the CHANGELOG? Thanks for the contribution!

? `--api-versions={${cfg.apiVersions
.map(apiVersion => shell.quote([apiVersion]))
.join(',')}}`
: `--api-versions=${cfg.apiVersions
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the length would only be 1 here, so how about

`---api-versions=${shell.quote(cfg.apiVersions[0])}`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. shell.quote takes a string[] so I just passed in apiVersions.

@lblackstone lblackstone mentioned this pull request Jan 15, 2020
@lblackstone
Copy link
Member

Merged these changes in in #951

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically fill in .Capabilities in Helm Charts
3 participants