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

make resourcequota and priorityClassName's optional #1425

Merged
merged 2 commits into from Jul 14, 2021
Merged

make resourcequota and priorityClassName's optional #1425

merged 2 commits into from Jul 14, 2021

Conversation

developer-guy
Copy link
Contributor

Signed-off-by: Batuhan Apaydın batuhan.apaydin@trendyol.com

What this PR does / why we need it:

This PR aims to provide conditional logic for resource ResourceQuota and field priorityClassName for both audit and controller-manager deployment resource for Gatekeeper.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #1421

Special notes for your reviewer:

@developer-guy
Copy link
Contributor Author

cc: @Dentrax @sozercan

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

A couple comments

}

if name == "gatekeeper-controller-manager" && kind == DeploymentKind {
obj = strings.Replace(obj, " priorityClassName: system-cluster-critical", " {{- if .Values.resourceQuota }} \n priorityClassName: {{ .Values.controllerManager.priorityClassName }}\n {{- end }}", 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be gated on values.controllerManager.priortyClassName, and similar for audit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you so much, it is my bad 🤦🏻‍♂️ it should be values.audit.priorityClassName. I fixed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still seeing a test against resourceQuota?: if .Values.resourceQuota

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this time, it should be ok @maxsmythe. 🙏

cmd/build/helmify/main.go Outdated Show resolved Hide resolved
@developer-guy
Copy link
Contributor Author

Thank you for the PR!

A couple comments

I updated the code according to your reviews @maxsmythe, thanks for helping me to fix the issues. 🙏

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM!

@sozercan LGTY?

Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

LGTM

@sozercan sozercan merged commit bc22879 into open-policy-agent:master Jul 14, 2021
@developer-guy
Copy link
Contributor Author

How can we include this PR in the next milestones @sozercan? I just want to know the version when this PR gets released.

julianKatz pushed a commit to julianKatz/gatekeeper that referenced this pull request Jul 27, 2021
…t#1425)

Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com>
Signed-off-by: juliankatz <juliankatz@google.com>
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.

with system-cluster-critical priorityClass is not permitted in gatekeeper-system
3 participants