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

fix: cleanup hooks not receiving error signal #1087

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

felipecrs
Copy link
Contributor

Closes #1041

@felipecrs
Copy link
Contributor Author

Any feedback on the approach I took is appreciated. Note that I am only interested on errors thrown in "sync" and "apply". For the other commands, if you believe it would be better to always set err as nil, I'm fine too.

@yxxhero
Copy link
Member

yxxhero commented Oct 23, 2023

@felipecrs could you add some tests for this feature? please ping me if you need any help.

@felipecrs
Copy link
Contributor Author

Sure, I'll see what I can do. BTW I am considering this a fix rather than a feature. :)

@yxxhero
Copy link
Member

yxxhero commented Sep 29, 2024

@felipecrs sorry for the ping. any updates? thanks so much.

@yxxhero
Copy link
Member

yxxhero commented Sep 29, 2024

This pull request includes several changes to the TriggerCleanupEvent method in the pkg/app/app.go and pkg/state/state.go files. The changes primarily focus on modifying the method to accept an additional error parameter (evtErr) and updating its calls accordingly.

Updates to TriggerCleanupEvent method:

This reverts commit d77c72f.

Signed-off-by: Felipe Santos <[email protected]>
@felipecrs
Copy link
Contributor Author

felipecrs commented Sep 29, 2024

@yxxhero the approach I was using was not being effective.

For some reason, the cleanup hook was being triggered twice, one through the functions I had modified in d77c72f, and another through 47b41fb.

However, even though both were being called, the first call exits early and doesn't get to execute the hook.

With that in mind, I reverted the changes made in d77c72f.

My question is: if the hook doesn't get executed through the calls of d77c72f, I wonder these calls exist in the first place.

Anyway, it's now working. I'm testing with the following:

hooks:
  - name: global-level
    events:
      - cleanup
    showlogs: true
    command: echo
    args:
      - "error is '{{`{{.Event.Error}}` | not | not}}'"

releases:
  - name: raw
    chart: https://github.com/dysnix/charts/releases/download/raw-v0.3.2/raw-v0.3.2.tgz
    wait: true
    timeout: 5
    values:
      - resources:
          - apiVersion: v1
            kind: Pod
            metadata:
              name: raw
            spec:
              containers:
                - name: raw
                  image: non-existing-image
    hooks:
      - name: release-level
        events:
          - postsync
        showlogs: true
        command: echo
        args:
          - "error is '{{`{{.Event.Error}}` | not | not}}'"

Previously:

$ helmfile sync
[...]
hook[postsync] logs | error is 'true'
[...]
hook[cleanup] logs | error is 'false'
[...]

Now:

$ helmfile sync
Upgrading release=raw, chart=/home/felipecrs/.cache/helmfile/https_github_com/dysnix/charts/releases/download/raw-v0.3.2/raw-v0.3.2.tgz, namespace=

hook[postsync] logs | error is 'true'
hook[postsync] logs | 

FAILED RELEASES:
NAME   NAMESPACE   CHART                                                                          VERSION   DURATION
raw                https://github.com/dysnix/charts/releases/download/raw-v0.3.2/raw-v0.3.2.tgz                   5s


hook[cleanup] logs | error is 'true'
hook[cleanup] logs | 
in ./helmfile.yaml.gotmpl: failed processing release raw: command "/home/linuxbrew/.linuxbrew/bin/helm" exited with non-zero status:

PATH:
  /home/linuxbrew/.linuxbrew/bin/helm

ARGS:
  0: helm (4 bytes)
  1: upgrade (7 bytes)
  2: --install (9 bytes)
  3: raw (3 bytes)
  4: /home/felipecrs/.cache/helmfile/https_github_com/dysnix/charts/releases/download/raw-v0.3.2/raw-v0.3.2.tgz (106 bytes)
  5: --wait (6 bytes)
  6: --timeout (9 bytes)
  7: 5s (2 bytes)
  8: --create-namespace (18 bytes)
  9: --values (8 bytes)
  10: /tmp/helmfile1744551334/raw-values-869f5446cb (45 bytes)
  11: --reset-values (14 bytes)
  12: --history-max (13 bytes)
  13: 10 (2 bytes)

ERROR:
  exit status 1

EXIT STATUS
  1

STDERR:
  Error: UPGRADE FAILED: context deadline exceeded

COMBINED OUTPUT:
  Error: UPGRADE FAILED: context deadline exceeded

@felipecrs
Copy link
Contributor Author

@felipecrs could you add some tests for this feature? please ping me if you need any help.

Can you help me with that? I am modifying run.go, and there are no existing tests for this file.

@felipecrs felipecrs marked this pull request as ready for review September 29, 2024 16:08
@felipecrs felipecrs requested a review from zhaque44 as a code owner September 29, 2024 16:08
@yxxhero
Copy link
Member

yxxhero commented Sep 30, 2024

@felipecrs I will help you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.Event.Error is not populated for cleanup global hooks
2 participants