-
Notifications
You must be signed in to change notification settings - Fork 776
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: Ident podLabels on deployments #3153
Conversation
During [this change](https://github.com/open-policy-agent/gatekeeper/pull/2788/files#diff-8418507fa48b71204028eeb671810c5e5c5e999cb23af3bbdceb06bc26bdc92dL39) the indentation was removed from the template but not added to the deployment manifest. Signed-off-by: Joao Pedro Silva <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -137,7 +137,7 @@ func (ks *kindSet) Write() error { | |||
} | |||
|
|||
if kind == DeploymentKind { | |||
obj = strings.Replace(obj, " labels:", " labels:\n{{- include \"gatekeeper.podLabels\" . }}", 1) | |||
obj = strings.Replace(obj, " labels:", " labels:\n{{- include \"gatekeeper.podLabels\" . | nindent 8 }}", 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obj = strings.Replace(obj, " labels:", " labels:\n{{- include \"gatekeeper.podLabels\" . | nindent 8 }}", 1) | |
obj = strings.Replace(obj, " labels:", " labels:\n {{- include \"gatekeeper.podLabels\" . | nindent 8 }}", 1) |
based on #3145 (comment) isn't this missing here to fix the intendation of the templating var?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indent is not needed as the function indents the whole block. You can test it on the template.
I believe the space would be removed by the -
at the beginning of the template block so it will not change anything.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3153 +/- ##
==========================================
+ Coverage 53.73% 53.78% +0.04%
==========================================
Files 136 136
Lines 12198 12198
==========================================
+ Hits 6555 6561 +6
+ Misses 5140 5136 -4
+ Partials 503 501 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
when is this change likely to be included in a release? |
What this PR does / why we need it:
During this
change the indentation was removed from the template but not added to the deployment manifest.
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 #3145
Special notes for your reviewer: