Make jib-gradle and jib-maven future-proof regarding Tekton $HOME deprecation #407
Conversation
tekton-robot
added
the
size/M
|
/assign @sbwsg |
jib-gradle/jib-gradle.yaml
Outdated
| -Djib.allowInsecureRegistries=$(params.INSECUREREGISTRY) \ | ||
| -Djib.to.image=$(resources.outputs.image.url) | ||
| workingDir: $(workspaces.source.path)/$(params.DIRECTORY) | ||
| env: | ||
| - name: HOME | ||
| value: /workspace |
There was a problem hiding this comment.
any information on why /workspace?
and what is the ${workspaces.source.path} in workingDir (line 49)?
There was a problem hiding this comment.
/workspace (the filesystem directory) is the directory that the current Tekton releases use as a general space for work. Tekton docs do specify this directory in their examples.
source in $(workspaces.source.path) is the name defined in line 17. There is a "Workspace" concept in Tekton, and source is a name given to a "Workspace". $(workspaces.source.path) evaluates to the path of this "Workspace". In our Task definition, this "source workspace" is where this Task should find Java source to containerize, hence setting the current working directory to it. In practice, this directory is /workspace/source.
tekton-robot
added
the
approved
Fantastic, thanks for testing that out! 👍 |
tekton-robot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: loosebazooka, sbwsg 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 |
tekton-robot
removed
the
needs-rebase
There was a problem hiding this comment.
So, if we follow the TEP-003 closely, this might be worth a 0.2 version. That said, the TEP is still in proposed phase, and those were just added so.. I think we can merge as is.
|
@sbwsg when can I expect this to be merged? |
|
I've approved it but it needs someone else to look it over and give it an lgtm. @loosebazooka , @briandealwis can both do that I think if I'm reading https://github.com/tektoncd/catalog/blob/master/task/jib-gradle/0.1/OWNERS right |
|
/lgtm |
|
So now I have to merge it? |
|
The tekton-bot should merge it automatically I believe. |
|
Hm, this PR is in the tide merge pool but it appears to be having some trouble: https://prow.tekton.dev/tide-history?repo=tektoncd%2Fcatalog&branch=master
|
|
@chanseokoh can you try manually rebasing on top of the master branch and pushing again? |
|
Re-adding the recent lgtm label /lgtm |
|
/lgtm |
|
Merge failing for the same reason . I only merged |
- Set HOME env to /workspace - Use $HOME for "home" wherever possible - Gradle: mount empty dir at ~/.gradle to make it globally writable - Maven: script to `script:` to use "$HOME" variable
|
/lgtm |
Changes
@sbwsg
As Tekton prepares for
/tekton/homedeprecation (with thedisable-home-env-overwriteanddisable-working-directory-overwriteswitches), these changes makejib-gradleandjib-maventasks compatible with the current Tekton release and future releases that will flip the switches.$HOMEenv to/workspaceat the Task level. (Luckily, this works even ifdisable-home-env-overwrite=false.)$HOMEfor accessing the home directory wherever possible$HOME/.gradleto make it globally writable (context: TaskRun fails during initialization when disable-home-env-overwrite=true pipeline#2165 (comment))command:toscript:to use$HOMEvariableI've verified these work before and after flipping the deprecation switches.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
There are yamllint violations mostly due to different array element indentation, but I decided not to take care of this after finding out it makes really difficult to diff changes on GitHub.
See the contribution guide
for more details.