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

Update json-patch mod to fix hangs on pulumi update #1223

Merged
merged 2 commits into from
Aug 1, 2020

Conversation

lblackstone
Copy link
Member

@lblackstone lblackstone commented Jul 27, 2020

Proposed changes

A performance bug was discovered that can cause CRD
diffs to fail. This bug is fixed in an open PR [1] to the upstream
repo, but has not been merged yet. In the meantime, I forked
the json-patch repo, and applied the fix [2]. This change will be
reverted once the upstream patch merges, and the module will
be updated to point at the fixed changeset.

[1] evanphx/json-patch#108
[2] https://github.com/pulumi/json-patch

Related issues (optional)

Fix #1222

@ninja-
Copy link

ninja- commented Jul 27, 2020

So as I understand, it would still fail during apply because helm cli is being invoked?

@ninja-
Copy link

ninja- commented Jul 27, 2020

The code that calls the CLI looks pretty simple to port, I know it was somewhere on the roadmap but what were the challenges there? I could spend some time working to get helm cli out of the way and use the go lib.
As I understand more or less, helm cli is just used to fetch and render templates into yaml and then manifests are passed into normal kubernetes engine of pulumi?

@lblackstone
Copy link
Member Author

The code that calls the CLI looks pretty simple to port, I know it was somewhere on the roadmap but what were the challenges there? I could spend some time working to get helm cli out of the way and use the go lib.

I’d be glad to move Helm support to use the library, but I don’t think it’s directly related to this issue since the diffs are handled by the provider.

I’d expect it to be fairly straightforward to port. The main issue would be supporting all of the existing config flags. Kustomise support is implemented using the library, so that’s a good example of what the work would look like.

As I understand more or less, helm cli is just used to fetch and render templates into yaml and then manifests are passed into normal kubernetes engine of pulumi?

Right; we only use the CLI to fetch the Chart and template YAML

A performance bug was discovered that can cause CRD
diffs to fail. This bug is fixed in an open PR [1] to the upstream
repo, but has not been merged yet. In the meantime, I forked
the json-patch repo, and applied the fix [2]. This change will be
reverted once the upstream patch merges, and the module will
be updated to point at the fixed changeset.

[1] evanphx/json-patch#108
[2] https://github.com/pulumi/json-patch
@lblackstone lblackstone force-pushed the lblackstone/upgrade-json-patch branch from 82055fb to 1eb3d34 Compare July 31, 2020 23:16
@lblackstone lblackstone marked this pull request as ready for review July 31, 2020 23:17
@lblackstone lblackstone changed the title Update json-patch module to v5 Update json-patch mod to fix hangs on pulumi update Jul 31, 2020
Copy link
Contributor

@metral metral left a comment

Choose a reason for hiding this comment

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

Is this a temp move until upstream is patched? Or are we considering forking long-term?

@lblackstone
Copy link
Member Author

lblackstone commented Jul 31, 2020

Is this a temp move until upstream is patched? Or are we considering forking long-term?

This change will be reverted once the upstream patch merges, and the module will be updated to point at the fixed changeset.

@lblackstone lblackstone merged commit 39fdc97 into master Aug 1, 2020
@pulumi-bot pulumi-bot deleted the lblackstone/upgrade-json-patch branch August 1, 2020 00:54
lblackstone added a commit that referenced this pull request Aug 19, 2020
Fixes have merged upstream, so move back
to the upstream dependency rather than our
fork.

Related: #1223
lblackstone added a commit that referenced this pull request Aug 19, 2020
Fixes have merged upstream, so move back
to the upstream dependency rather than our
fork.

Related: #1223
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.

Installing cert-manager with CRDs using helm hangs pulumi
3 participants