-
Notifications
You must be signed in to change notification settings - Fork 115
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
Fix import handling for Helm Release #1818
Conversation
I have manually verified this. Working on an integration test for this. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
1 similar comment
Does the PR have any schema changes?Looking good! No breaking changes found. |
I think this looks good, but it will likely have a merge conflict with #1809 Not sure which would be easier to merge first |
We tried out that PR with the release we're trying to import and we get a warning during preview:
and equivalent error during the actual import:
Is there anything we should do to make that work? Our resource looks like this in TS:
|
Thanks for testing this out already! I discovered this and a couple of similar issues in my tests as well. Essentially we need to drop some computed items from the inputs which are being detected as conflicts. In addition, we set certain fields as defaults which are conflicting with the import. I will have fixes for both in the PR shortly. |
f20b948
to
459d43f
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
1 similar comment
Does the PR have any schema changes?Looking good! No breaking changes found. |
@@ -647,21 +647,19 @@ func (k *kubeProvider) Configure(_ context.Context, req *pulumirpc.ConfigureRequ | |||
// Attempt to load the configuration from the provided kubeconfig. If this fails, mark the cluster as unreachable. | |||
if !k.clusterUnreachable { | |||
config, err := kubeconfig.ClientConfig() | |||
|
|||
if kubeClientSettings.Burst != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into some panics here in tests which this change fixed.
@@ -0,0 +1,21 @@ | |||
# Patterns to ignore when building packages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as the chart in the local chart example. Its a lot easier to refer to a local chart with the manual chart install and figured its best to copy it. Let me know if we should consolidate instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me
Does the PR have any schema changes?Looking good! No breaking changes found. |
@lblackstone I think this is ready for another look now. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
@@ -0,0 +1,21 @@ | |||
# Patterns to ignore when building packages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me
Does the PR have any schema changes?Looking good! No breaking changes found. |
First, thank you so much @viveklak for taking the time to add support for imports, that's deeply appreciated! 👍 Now, I have tried to import our release again with a build of the
Is there anything we're not doing correctly on our end or is it just yet another computed input that would need to be dropped by the provider? |
(Sorry, that was me above, just logged as our bot account! 😉) |
Another input that seems to be computed dynamically and which should maybe be dropped is |
@silphid thanks for that. You might want to disable the following from the resource during the import.
The reason for this is that helm doesn't seem to persist the repository information on the release. You may want to add You can also drop Happy to chat over slack if you run into further issues with the above and huge thanks for taking this out for a spin so quickly! |
Proposed changes
Adds ability to import releases.
Note - since releases don't themselves record information about the chart used, repository information is omitted from the code generated.
Related issues (optional)
#1698