fix: HOOKS section should also be parsed when HELM_DIFF_USE_UPGRADE_DRY_RUN=true #364
Conversation
cmd/helm3.go
Outdated
| @@ -175,14 +175,25 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) { | |||
| return s | |||
| } | |||
|
|
|||
| i := bytes.Index(s, []byte("MANIFEST:")) | |||
| i := bytes.Index(s, []byte("HOOKS:")) | |||
| s = s[i:] | |||
| i = bytes.Index(s, []byte("---")) | |||
There was a problem hiding this comment.
@carlosrmendes Hey! Sorry for the delay. I was reviewing this change carefully and this seems not working as expected when your chart doesn't have hooks at all.
See this dry-run output from a clean install of stable/envoy chart:
NAME: my1
LAST DEPLOYED: Sun Feb 13 01:04:52 2022
NAMESPACE: default
STATUS: pending-upgrade
REVISION: 2
TEST SUITE: None
HOOKS:
MANIFEST:
---
# Source: envoy/templates/poddisruptionbudget.yaml
apiVersion: policy/v1beta1
kind: PodDisruptionBudget
metadata:
This bytes.Index call at 180 would return the index of --- in
MANIFEST:
---
which isn't correct, and perhaps that's why you had to add complex nested-if statements in L183-L191, right?
Probably a better way for us would be to treat --- for HOOKS section as optional, instead of that for MANIFEST.
There was a problem hiding this comment.
I also noticed that we had to add support for --no-hooks (and --include-tests), and it ended up with 3d4ced1. I hope you like it 😄
There was a problem hiding this comment.
This seems good to go!
Although I added some changes, the whole work for this wasn't possible with your contribution! 99% credit goes to you- Thank you so much for your contribution @carlosrmendes
mumoshu
changed the title
resolves #363