Point $HOME to /tekton/home #1628
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
the
size/L
imjasonh
force-pushed
the
new-home-dir
branch
from
1df2a19
to
581f068
Compare
tekton-robot
added
do-not-merge/hold
There was a problem hiding this comment.
yesssss thanks for all the doc updates 🎉
This seems reasonable to me! I could see someone relying on $HOME but also hardcoding /builder/home in the same Task but regardless this seems like a reasonable path forward to me.
Just a couple quick questions about things I'm a bit confused by.
/approve
| args: ['-c', '[[ ! -f /builder/home/stuff ]]'] | ||
| volumeMounts: | ||
| - name: empty | ||
| mountPath: /builder/home |
There was a problem hiding this comment.
question for my own self reasons (aka #1438 ) : how useful do you find it to be able to declare volumeMounts for every step and is this something you'd want if you could define a volumeMount for the entire Task instead?
There was a problem hiding this comment.
I think that's possible today with the step template
There was a problem hiding this comment.
Yeah, I think that's what stepTemplate and task.spec.volumes is for.
There was a problem hiding this comment.
I mean it's possible but do you feel like you want to do it - i.e. would you be happy with just having a volume that is mounted for the entire Task and volumeMounts on a step didnt work?
if im not making any sense then just ignore me and ill ask you both again once I have an example :D
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobcatfish, vdemeester 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 |
imjasonh
force-pushed
the
new-home-dir
branch
from
581f068
to
e402420
Compare
| @@ -90,7 +90,7 @@ spec: | |||
| # specifying DOCKER_CONFIG is required to allow kaniko to detect docker credential | |||
| env: | |||
| - name: "DOCKER_CONFIG" | |||
| value: "/builder/home/.docker/" | |||
| value: "/tekton/home/.docker/" | |||
There was a problem hiding this comment.
I think it would be nicer to use $HOME/.docker here - so we don't promote relying on a specific path
There was a problem hiding this comment.
Unfortunately, env var values can't be based on other existing env var values. :(
You need a shell involved to actually resolve env vars.
We could do some silly nonsense like replacing the string $HOME in env var values before we send it to the Pod, but that feels like a confusing change from K8s behavior that I'd like to avoid.
There was a problem hiding this comment.
Oh, good point, I see. This is really a pity though.
It might be worth having a built-in variable for that, but I would use Tekton syntax to avoid confusion:
env:
- name: "DOCKER_CONFIG"
value: "$(home)/.docker/"
/builder/home is still mounted, backed by the same volume, to allow users some time to move over before we move /builder/home in v0.10 and beta.
imjasonh
force-pushed
the
new-home-dir
branch
from
e402420
to
138db46
Compare
|
/lgtm |
|
/hold cancel |
tekton-robot
removed
the
do-not-merge/hold
| @@ -105,7 +105,7 @@ spec: | |||
| DATE=$(date +"%Y%m%d") | |||
| VERSION_TAG="$DATE-$COMMIT" | |||
|
|
|||
| echo $VERSION_TAG > "/builder/home/version" | |||
| echo $VERSION_TAG > "/tekton/home/version" | |||
There was a problem hiding this comment.
I just realized that since this runs on Tekton 0.3.1, there will be no /tekton/home there :(
This was changes in tektoncd#1628, however since this task runs on Tekton 0.3.1, we need to stick with /builder/home there, at least until we get rid of Tekton 0.3.1 in the Prow cluster.
This was changes in #1628, however since this task runs on Tekton 0.3.1, we need to stick with /builder/home there, at least until we get rid of Tekton 0.3.1 in the Prow cluster.
/builder/homeis still mounted, backed by the same volume, to allowusers some time to move over before we move /builder/home in v0.10 and
beta.
More progress toward #1030
This is step 1 of a breaking change, since some users might unfortunately rely on the specific path that we document. We'll warn people in v0.9 release notes that the path is changing, and remove the old path in v0.10.
Users who rely on
$HOMEwill not be affected (that points to/tekton/homeafter this change)Changes
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes