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

feat: enable configuration for finalizer removal if artifact GC fails #10810

Merged
merged 14 commits into from
Apr 7, 2023

Conversation

juliev0
Copy link
Contributor

@juliev0 juliev0 commented Apr 4, 2023

Fixes #10788

Why: some users seem to want their Workflows to be deletable when Artifact GC fails (through other means besides argo delete --force). The idea is to make it something that's configurable.

What:

I considered:

  • making it configurable in the ArtifactGC struct (which lives on the Workflow and artifact level)
  • making it configurable only on the Workflow level
  • making it configurable in the configmap somewhere

I tried both the 1st and 2nd options. The 1st was a possibility but it seemed to probably be unnecessary in that users won't need different configurability for different artifacts. It also meant needing to make the field something other than a boolean, since otherwise you can't really distinguish between:

  • wfLevel: forceFinalizerRemoval=true, artifactLevel: forceFinalizerRemoval=false, and
  • wfLevel: forceFinalizerRemoval=true, artifactLevel: forceFinalizerRemoval not set

I went with the 2nd option.

Testing: Expanded the e2e test to test whether the finalizer was removed when ArtifactGC failed both when forceFinalizerRemoval was set to true and when it was set to false.

Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
@juliev0 juliev0 closed this Apr 4, 2023
@juliev0 juliev0 reopened this Apr 4, 2023
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
@juliev0 juliev0 marked this pull request as ready for review April 6, 2023 04:03
@juliev0 juliev0 marked this pull request as draft April 6, 2023 04:09
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
@juliev0 juliev0 marked this pull request as ready for review April 6, 2023 04:24
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Signed-off-by: Julie Vogelmani <julie_vogelman@intuit.com>
Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

LGTM.

@terrytangyuan terrytangyuan merged commit e734ae5 into argoproj:master Apr 7, 2023
JPZ13 pushed a commit to pipekit/argo-workflows that referenced this pull request Jul 4, 2023
@agilgur5 agilgur5 added the area/artifacts S3/GCP/OSS/Git/HDFS etc label Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/artifacts S3/GCP/OSS/Git/HDFS etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable the option of Workflows being deleted when Artifact GC fails
4 participants