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

Move YAML decode logic into provider #925

Merged
merged 11 commits into from
Jan 6, 2020
Merged

Move YAML decode logic into provider #925

merged 11 commits into from
Jan 6, 2020

Conversation

lblackstone
Copy link
Member

@lblackstone lblackstone commented Dec 17, 2019 •
edited
Loading

Proposed changes

Use a runtime invoke to call a common decodeYaml method in the
provider rather than using YAML libraries specific to each language.

Related issues (optional)

#865

@lblackstone
Move YAML decode logic into provider
7113047 
Use a runtime invoke to call a common decodeYaml method in the
provider rather than using YAML libraries specific to each language.
lukehoban
lukehoban reviewed Dec 17, 2019
View reviewed changes
sdk/nodejs/yaml/yaml.ts Outdated Show resolved Hide resolved
sdk/nodejs/yaml/yaml.ts Outdated Show resolved Hide resolved
sdk/nodejs/yaml/yaml.ts Outdated Show resolved Hide resolved
sdk/nodejs/yaml/yaml.ts Outdated Show resolved Hide resolved
@lblackstone
Copy link
Member Author

lblackstone commented Dec 18, 2019 •
edited
Loading

Tests passed, and performance appears to be better than the attempt in #908. The CI run took about 3 mins longer than usual (approx. 10% increase), which likely indicates a modest performance hit from using invoke in the preview/update path.

Edit: Actually, it appears that the increased time is related to the recent .NET additions rather than a performance regression.

pgavlin
pgavlin reviewed Jan 6, 2020
View reviewed changes
pkg/provider/invoke_decode_yaml.go Outdated Show resolved Hide resolved
pkg/provider/invoke_decode_yaml.go Show resolved Hide resolved
@lblackstone @pgavlin
Update pkg/provider/invoke_decode_yaml.go
cf418f0 
Co-Authored-By: Pat Gavlin <pat@pulumi.com>
@lblackstone lblackstone force-pushed the lblackstone/invoke-yaml-decode branch from 044a0f0 to cf418f0 Compare January 6, 2020 21:18
@lblackstone lblackstone merged commit c75f754 into master Jan 6, 2020
@pulumi-bot pulumi-bot deleted the lblackstone/invoke-yaml-decode branch January 6, 2020 22:34
@lblackstone lblackstone mentioned this pull request Jan 7, 2020
Closed
lblackstone added a commit that referenced this pull request Jan 8, 2020
@lblackstone
Revert "Move YAML decode logic into provider (#925)"
b47f9d1 
This reverts commit c75f754.
lblackstone added a commit that referenced this pull request Jan 8, 2020
@lblackstone
Revert invoke changes (#941)
58d4ccc 
* Revert "Improve handling of default namespace for Helm charts (#934)"

This reverts commit 2652b10.

* Revert "Move YAML decode logic into provider (#925)"

This reverts commit c75f754.
lblackstone added a commit that referenced this pull request Jan 16, 2020
@lblackstone
Move YAML decode logic into provider and improve default Helm namespaces
489ad79 
Reintroduce the reverted changed (#941) from #925 and #934 with a few
additional fixes related to the changes in #946.

The major changes include the following:

- Use a runtime invoke to call a common decodeYaml method in the
provider rather than using YAML libraries specific to each language.
- Use the namespace parameter of helm.v2.Chart as a default,
and set it on known namespace-scoped resources.
@lblackstone lblackstone mentioned this pull request Jan 16, 2020
Merged
lblackstone added a commit that referenced this pull request Jan 16, 2020
@lblackstone
Move YAML decode logic into provider and improve default Helm namespaces
84e8e22 
Reintroduce the reverted changed (#941) from #925 and #934 with a few
additional fixes related to the changes in #946.

The major changes include the following:

- Use a runtime invoke to call a common decodeYaml method in the
provider rather than using YAML libraries specific to each language.
- Use the namespace parameter of helm.v2.Chart as a default,
and set it on known namespace-scoped resources.
lblackstone added a commit that referenced this pull request Jan 21, 2020
@lblackstone
Move YAML decode logic into provider and improve default Helm namespa…
af7af1c 
…ces (#952)

Reintroduce the reverted changed (#941) from #925 and #934 with a few
additional fixes related to the changes in #946.

The major changes include the following:

- Use a runtime invoke to call a common decodeYaml method in the
provider rather than using YAML libraries specific to each language.
- Use the namespace parameter of helm.v2.Chart as a default,
and set it on known namespace-scoped resources.
@nesl247 nesl247 mentioned this pull request Feb 4, 2020
Closed
@renovate renovate bot mentioned this pull request Jun 6, 2021
Open
1 task
@just1900
Copy link

just1900 commented Jun 15, 2022 •
edited
Loading

Hi @lblackstone, sorry to bother here, I just run into a problem of helm chart ordering like #239 , it seems that this PR removed helmSort function, wondering what's the best practice now to keep the chart resources order like how helm do?

@lblackstone
Copy link
Member Author

@just1900 pulumi-kubernetes automatically retries resource creation on failure, so normally the ordering isn't very important. We removed the sorting to simplify the logic, and it seems to have worked well since that time.

Can you open a new issue with more details if you're still having problems?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgavlin pgavlin pgavlin approved these changes

@metral metral Awaiting requested review from metral

@lukehoban lukehoban Awaiting requested review from lukehoban

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lblackstone @just1900 @lukehoban @pgavlin