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: Hook Deletion Policies HookSucceeded should be run after whole H… #144

Merged
merged 3 commits into from
Oct 9, 2020

Conversation

mayzhang2000
Copy link
Contributor

…ook succeed and not only Resource succeed

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2020

Codecov Report

Merging #144 into master will increase coverage by 0.42%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
+ Coverage   53.86%   54.29%   +0.42%     
==========================================
  Files          25       25              
  Lines        2404     2422      +18     
==========================================
+ Hits         1295     1315      +20     
+ Misses        974      972       -2     
  Partials      135      135              
Impacted Files Coverage Δ
pkg/sync/sync_context.go 70.97% <75.00%> (+0.61%) ⬆️
pkg/sync/sync_task.go 100.00% <100.00%> (+2.63%) ⬆️
pkg/cache/cluster.go 49.87% <0.00%> (+1.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7171d62...1623488. Read the comment docs.

@mayzhang2000 mayzhang2000 requested a review from alexmt September 23, 2020 21:05
Copy link
Contributor

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

Tried to test it and found a bug. Following pre-sync hook has HookFailed delete policy, always fails but never got deleted.

apiVersion: batch/v1
kind: Job
metadata:
  name: before
  annotations:
    argocd.argoproj.io/hook: PreSync
    argocd.argoproj.io/hook-delete-policy: HookFailed
spec:
  template:
    spec:
      containers:
      - name: sleep
        image: alpine:latest
        command: ["fail"]
      restartPolicy: Never
  backoffLimit: 0

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 1, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

No Coverage information No Coverage information
8.3% 8.3% Duplication

Copy link
Contributor

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

LGTM

@mayzhang2000 mayzhang2000 merged commit 54bbebf into argoproj:master Oct 9, 2020
@mayzhang2000
Copy link
Contributor Author

fixes argoproj/argo-cd#4384

mayzhang2000 added a commit that referenced this pull request Oct 29, 2020
#144)

* fix: Hook Deletion Policies HookSucceeded should be run after whole Hook succeed and not only Resource succeed

* fix: handle HookFailed.

* fix: fixing lint error.
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.

3 participants