Skip to content

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!

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