[kjobctl] Move unmask_filename to templates. #3180
Conversation
k8s-ci-robot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mbobrovskyi The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
mbobrovskyi
force-pushed
the
cleanup/move-unmask-filename-function-to-templates
branch
from
6fd8f26
to
24c188a
Compare
|
/cc @trasc |
k8s-ci-robot
added
the
needs-rebase
mbobrovskyi
force-pushed
the
cleanup/move-unmask-filename-function-to-templates
branch
from
24c188a
to
370eafd
Compare
k8s-ci-robot
removed
the
needs-rebase
k8s-ci-robot
added
size/L
mbobrovskyi
changed the title
unmask_filename to templates.unmask_filename as embed file.
mbobrovskyi
force-pushed
the
cleanup/move-unmask-filename-function-to-templates
branch
from
387f031
to
5d71e96
Compare
There was a problem hiding this comment.
this is not a .tmpl :) , it should not be a match for the template wildcard
There was a problem hiding this comment.
The issue is that the linter incorrectly identifies it as a .sh file, resulting in an error: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kueue/3180/pull-kueue-test-kjobctl/1842216444394213376. Adding a shebang here results in it being duplicated in the template, which is incorrect. We could also ignore it using # shellcheck disable=SC2148, but this would make the resulting output less clear.
Honestly, I think it should be template because this is a part of template. I moved it to separate file to be easier test this function. I don't see significant advantages to not using it as a template, but there are drawbacks - such as developer comments being included in the result and we need to create a separate embed variable, whereas simply including the file name in the template would be much easier.
I propose reverting to the previous solution, which I believe is much better. However, if you have another solution that you think is better, feel free to discuss it.
There was a problem hiding this comment.
Since it is only used in one place you could just put its content in the entrypoint template.
There was a problem hiding this comment.
We can't test it in this way 😕.
| replaced="${replaced//%%/%}" | ||
|
|
||
| echo "$replaced" | ||
| } |
There was a problem hiding this comment.
NIT:
| } | |
| } | |
There was a problem hiding this comment.
Why do we need it? This space will be added in the main template.
mbobrovskyi
force-pushed
the
cleanup/move-unmask-filename-function-to-templates
branch
from
5d71e96
to
2168679
Compare
k8s-ci-robot
added
size/M
mbobrovskyi
changed the title
unmask_filename as embed file.
|
We discussed this case wit @trasc and take a decision to continue working on it after implementing e2e tests. /hold |
k8s-ci-robot
added
the
do-not-merge/hold
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Move unmask_filename to templates in kjobctl.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Follow up for #3136 (comment).
Does this PR introduce a user-facing change?