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

helm-diff doesn't include values from stdin for helm3 version #340

Merged
merged 1 commit into from
Jan 10, 2022

Conversation

ruslanloman
Copy link
Contributor

Hi,
Unfortunately previous PR was close as state, but this fix is still needed, so I've created a new one. Please review it once you have time. thanks!

There is an issue in helm-diff for helm3 version, it doesn't include values from stdin. For helm2 it works as expected.

$ cat values-10.yaml                                                               
lolkek:                                                                            
  helloword: test                                                                  
  new:                                                                             
    ohoho:                                                                         
      values-10: lolodddd10 

Current behavior:

$ cat values-10.yaml | helm3 diff upgrade test_helm . --allow-unreleased --values -
                                                                         
+ # Source: test_helm/templates/configMap.yaml                                     
+ apiVersion: v1                                                                   
+ kind: ConfigMap                                                                  
+ metadata:                                                                        
+   name: game-demo                                                                
+ data:                                                                            
+   user-interface.properties: |                                                   
+     null         

Expected behavior with applied fix:

$ cat values-10.yaml | helm3 diff upgrade test_helm . --allow-unreleased --values -
                                                                                 
+ # Source: test_helm/templates/configMap.yaml                                     
+ apiVersion: v1                                                                   
+ kind: ConfigMap                                                                  
+ metadata:                                                                        
+   name: game-demo                                                                
+ data:                                                                            
+   user-interface.properties: |                                                   
+     ohoho:

@ruslanloman
Copy link
Contributor Author

@mumoshu applied your changes. thanks!

cmd/helm3.go Outdated
Comment on lines 135 to 138
if _, err := tmpfile.Write(bytes); err != nil {
return nil, err
}
Copy link
Collaborator

@mumoshu mumoshu Jan 10, 2022

Choose a reason for hiding this comment

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

Did this change actually work for you?

I'm afraid we're missing tmpfile.Close() here, that I think is required to ensure writes are persisted. We should call Close() regardless of the write err is nil or not so wrapping it into another function call would be easier to read.

See writeExistingValues for your reference!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it worked pretty well. I got your point regarding closing the tempfile. tbh i don't want to overcomplicate it by adding one more function, so just added one more if condition.

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM. Thanks for your effort!

@ruslanloman ruslanloman force-pushed the helm3_stdin branch 2 times, most recently from 9d434e4 to ae2958a Compare January 10, 2022 16:20
Copy link
Collaborator

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

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

I appreciate your contribution ☺️

@mumoshu mumoshu merged commit cee80a5 into databus23:master Jan 10, 2022
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.

2 participants