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(constraint): add new argo-workflow-gc constraint #49

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MichaelPatsula
Copy link
Contributor

All Argo Workflows should have the podGC field set (https://argoproj.github.io/argo-workflows/fields/#podgc).

Copy link
Contributor

@justbert justbert left a comment

Choose a reason for hiding this comment

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

I think this is a good start. Have we thought about using a Mutation to automatically set the GC settings? This might be a good place to do it since it looks to be a pretty simple policy and there isn't too much to change. It could be interesting to have both the mutation and the policy in case, defense in depth. (mutations are run first, then the admission policy so it should work fine!)

Can you add some newline at the end of files? Just some lil' nitpicks :)

general/argo-workflow-gc/examples/constraint.yaml Outdated Show resolved Hide resolved
general/argo-workflow-gc/examples/constraint.yaml Outdated Show resolved Hide resolved
Comment on lines 20 to 27
package argoworkflowgc

violation[{"msg": msg}] {
input.review.kind.kind == "Workflow"
not input.review.object.spec.podGC

msg := sprintf("podGC field is required in an Argo Workflow manifest. %s", [input.parameters.errorMsgAdditionalDetails])
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the violation for CronWorkflows is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

@MichaelPatsula
Copy link
Contributor Author

I think this is a good start. Have we thought about using a Mutation to automatically set the GC settings? This might be a good place to do it since it looks to be a pretty simple policy and there isn't too much to change. It could be interesting to have both the mutation and the policy in case, defense in depth. (mutations are run first, then the admission policy so it should work fine!)

Can you add some newline at the end of files? Just some lil' nitpicks :)

Thanks for the reviewing the PR! Good point, I think the mutation idea sounds interesting & useful! I'll look into it.

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