Don't inject must-gather fields directly into job yaml #138
Conversation
openshift-ci
bot
added
the
approved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #138 +/- ##
=========================================
- Coverage 10.67% 7.83% -2.85%
=========================================
Files 4 5 +1
Lines 281 383 +102
=========================================
Hits 30 30
- Misses 248 350 +102
Partials 3 3
|
| return job, nil | ||
| } | ||
|
|
||
| func processJobTemplate(jobTemplate string, instance *mustgatherv1alpha1.MustGather) (*batchv1.Job, error) { |
There was a problem hiding this comment.
Do we need the jobTemplate YAML for anything? Should we just default values in the struct itself?
job := &batchv1.Job{
ObjectMeta: metav1.ObjectMeta{
Name: instance.ObjectMeta.Name,
Namespace: instance.ObjectMeta.Namespace,
},
Spec: {
// The rest of the default/non-default values
}There was a problem hiding this comment.
I'm fully on board with this approach, but it's more work. Long term I think it's better and am more than happy to go through with this.
| gatherCommandBinary = gatherCommandBinaryNoAudit | ||
| } | ||
|
|
||
| job.Spec.Template.Spec.Containers[0].Command = append( |
There was a problem hiding this comment.
For context to the earlier comment, I'm mostly concerned about the brittleness of these array indices if someone comes in and changes the template in some way. My concerns could also be addressed with unit tests that would fail if someone mucked with the jobTemplate's containers
There was a problem hiding this comment.
+1 this comment fully convinces me we should address this now. There's no guarantee of order in the unmarshalling.
|
/label tide/merge-method-squash |
openshift-ci
bot
added
the
tide/merge-method-squash
|
/lgtm cancel |
build/Dockerfile
Outdated
There was a problem hiding this comment.
We'll also need to take out this line
COPY --from=builder /src/build/templates /etc/templates
AlexVulaj
force-pushed
the
no-direct-user-input
branch
from
2549aeb to
4997ad9
Compare
|
@AlexVulaj: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlexVulaj, mjlshen The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This change prevents values from the MustGather resource from bleeding into other fields from the Job template.
Resolves OSD-21667