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

[sdk/nodejs] Add a ready attribute to await Helm Charts #1364

Merged
merged 8 commits into from
Nov 12, 2020

Conversation

lblackstone
Copy link
Member

@lblackstone lblackstone commented Oct 28, 2020

Proposed changes

After chatting with @pgavlin about this problem, we determined that Helm Charts were not awaiting properly because child resources are not known at the time the Chart is initialized. The most promising solution appears to be exposing a field on the Chart class that can be passed into the dependsOn option. The existing resources field is a map rather than an array, so we either need to expand dependsOn support to include maps, or create a new field on the Chart class that works with the existing definition.

This PR takes the latter approach, and creates a new .ready field that can be passed directly to the dependsOn option. This change makes dependent resources wait until all Chart subresources are ready as expected. Here's an example program to demonstrate:

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

const ns = new k8s.core.v1.Namespace("foo")
const chart = new k8s.helm.v3.Chart("nginx", {
    chart: "nginx",
    namespace: ns.metadata.name,
    values: {
        service: {
            type: "ClusterIP"
        }
    },
    fetchOpts: {
        repo: "https://charts.bitnami.com/bitnami"
    },
})

new k8s.core.v1.ConfigMap("test", {
    data: {foo: "bar"}
}, {dependsOn: chart.ready})

Here's what this looks like before/after the change (notice the test ConfigMap gets created immediately on Before):

Before

Before

After

After

Related issues (optional)

Related to #861

@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.

Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

Copy link
Member

@pgavlin pgavlin left a comment

Choose a reason for hiding this comment

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

This looks good to me. Any ideas on how to test this?

@mitchellmaler
Copy link

Hey @lblackstone what would the code look like for the python sdk, I am guessing very similar. Wanted to give it a try.

@lblackstone
Copy link
Member Author

Hey @lblackstone what would the code look like for the python sdk, I am guessing very similar. Wanted to give it a try.

I haven't had a chance to work on it yet, but should be the same basic concept. Easiest way to test out would be modifying the relevant Helm/YAML SDK files in your virtualenv

@lblackstone
Copy link
Member Author

Any ideas on how to test this?

I'm not sure yet. I'd like to verify that the ConfigMap isn't created until after all of the Chart resources, but I'm not sure how to get that information. I don't suppose there's an easy way to get timestamps of resource creation events? @pgavlin

@lblackstone lblackstone marked this pull request as ready for review November 11, 2020 19:10
@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.

Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@lblackstone
Copy link
Member Author

/run-acceptance-tests

@github-actions
Copy link

@lblackstone
Copy link
Member Author

Note: this PR is only for NodeJS. Once this merges, I'll follow with the other SDKs

@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.

Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.

Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@lblackstone
Copy link
Member Author

/run-acceptance-tests

@github-actions
Copy link

@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.

Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@@ -255,7 +255,7 @@ export class Chart extends yaml.CollectionComponentResource {
}
});

this.ready = this.resources.apply(m => Object.values(m).map(r => pulumi.output(r)));
this.ready = this.resources.apply(m => Object.values(m));
Copy link
Member

Choose a reason for hiding this comment

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

Ah nice, I was just gonna say the same thing ✅

@lblackstone
Copy link
Member Author

/run-acceptance-tests

@github-actions
Copy link

@lblackstone lblackstone merged commit 02cb470 into master Nov 12, 2020
@pulumi-bot pulumi-bot deleted the lblackstone/helm-await branch November 12, 2020 18:18
@EronWright EronWright changed the title Add a ready attribute to await Helm Charts [sdk/nodejs] Add a ready attribute to await Helm Charts Jun 17, 2024
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.

None yet

4 participants