Skip to content
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

Add Jib Gradle catalog / fix Docker config issue for Jib Maven #181

Merged
merged 2 commits into from
Feb 24, 2020
Merged

Add Jib Gradle catalog / fix Docker config issue for Jib Maven #181

merged 2 commits into from
Feb 24, 2020

Conversation

chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Feb 13, 2020

Changes

Add Jib Gradle catalog / fix Docker config issue for Jib Maven

(@GoogleContainerTools/java-tools-build FYI)

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.

@tekton-robot tekton-robot requested review from bobcatfish and a user February 13, 2020 22:15
@tekton-robot tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 13, 2020
@tekton-robot
Copy link

Hi @chanseokoh. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 13, 2020
@chmouel
Copy link
Member

chmouel commented Feb 14, 2020

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 14, 2020
Copy link
Member

@chmouel chmouel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to think about consolidating all related commands to jib/ ie jib/maven.yaml jib/gradle.yaml but that may be done next as a scleanup

jib-gradle/jib-gradle.yaml Outdated Show resolved Hide resolved
@chanseokoh
Copy link
Contributor Author

Any chance to get this reviewed?

# Runs the Gradle Jib build.
gradle jib \
--stacktrace --console=plain \
--init-script=/tekton/home/init-script.gradle \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should use /tekton/home directly in case if that change and with the work @sbwsg is planning to do, what do you think Scott?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, unless I'm misunderstanding the purpose I think the safest thing will be to use $HOME. It should be /tekton/home until we flip the default behaviour to be whatever the image expects to be HOME and then it should follow that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbwsg @chmouel I don't think I can use $HOME in the following locations. Knowing that, I'd rather use the /tekton/home literal, since at least things won't break even if $HOME changes under the hood. It will continue to work; it will be just that everyone will assume /tekton/home is a user home and use it.

    - name: $(inputs.params.CACHE)
      mountPath: /tekton/home/.gradle/caches
      subPath: gradle-caches
    - name: $(inputs.params.CACHE)
      mountPath: /tekton/home/.gradle/wrapper
      subPath: gradle-wrapper
    - name: $(inputs.params.CACHE)
      mountPath: /tekton/home/.m2
      subPath: m2-cache
    - name: $(inputs.params.CACHE)
      mountPath: /tekton/home/.cache
      subPath: jib-cache

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be an opportunity to use workspaces instead of raw volumes? You wouldn't need to pass a string volume name around as a param, users of your task would have the opportunity to pass PVCs or empty dirs as they see fit, and they could specify subPaths of those volumes if they wanted to share the caches on a single volume as is done here.

/tekton is generally intended as a reserved area for Tekton internals. Having it include a home directory seems, in hindsight, to be a mistake. I totally understand if you need a literal directory name to use but I don't recommend anything under /tekton.

Copy link
Contributor Author

@chanseokoh chanseokoh Feb 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, the user home must be /tekton/home in JVM, because Tekton auth creates a Docker config at /tekton/home/.docker/config.json. Unless Tekton fixes this or has an option to create that file on <actual home by image>/.docker/config.json, I have no choice but force /tekton/home as the home directory. Otherwise, Jib just cannot find this file.

(And if you haven't realized, when the user home is set to /tekton/home in JVM, Gradle and Maven write artifacts to /tekton/home/.m2, etc.)

Yeah, this is very unfortunate and has been pretty annoying as I said in tektoncd/pipeline#2013 (comment) I hope Tekton fixes the home issue quickly.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh dear. OK then. 👍

@chmouel
Copy link
Member

chmouel commented Feb 19, 2020

Fine by me, just need to make sure about /tekton/home if there is a better way to do this,

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 20, 2020
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2020
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot merged commit da6c78b into tektoncd:master Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants