Fix: determine whether the destination path already exists before clone

[yuzp1996]

Changes

If directory ${GOPATH}/src/github.com/google/licenseclassifier exists and is not empty we should skip clone again or will cause an error.

Related Issue: tektoncd/pipeline#5193

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes docs (if user facing)
• Commit messages follow commit message best practices

See the contribution guide
for more details.

[tekton-robot]

Hi @yuzp1996. 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.

[yuzp1996]

related Issue: tektoncd/pipeline#5193

[yuzp1996]

/kind bug

[yuzp1996]

I made the change to resolve the problem when running the e2e-upgrade-test.sh but I am not sure how to test this change.

I find that the file entrypoint.sh will be referenced here and will be copied to the image test-runner which is used to run the e2e upgrade test.

Looks like I need first build the test-runner image and replace the container image which runs the e2e upgrade test in the Prow job.

Is there any other test cluster to test this change? I am not sure of the step to test this change.

[lbernick]

Thank you @yuzp1996!
I'm not sure exactly how to test this but here's what I'd try (maybe @afrittoli would have some more suggestions?)
First build the docker image that includes this file:
docker build tekton/images/test-runner -t test-runner
Then try running the unit tests with your newly built image and see if you still run into the licenses issue:
docker run -v <path-to-your-pipelines-clone>:/pipeline test-runner /pipeline/test/presubmit-tests.sh --unit-tests

(I'm suggesting unit tests here because they also run with the test-runner image, but don't require creating a k8s cluster. I haven't tested these commands because I unfortunately don't have a working docker setup right now :( I'm pretty sure the other args for the prow job are args for prow, not for tests.)

If this works we'll have to update the prow job to use a newer version of this image once it's built. It looks like the version of test-runner it's using isn't what the rest of our CI uses and I'm wondering if just updating the image would help.

[abayer]

/test plumbing-image-build

[tekton-robot]

@abayer: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

• /test pull-dogfooding-mario-test
• /test pull-tekton-plumbing-check-testgrid-config

In response to this:

/test plumbing-image-build

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.

[abayer]

/approve

Yeah, @lbernick's suggestion should be a good way to verify the behavior. That said, I'm pretty sure it's good. =) Once it's merged and new images get built overnight, I'll update the Prow config to use the new tag.

[abayer]

/test plumbing-yamlllint

[tekton-robot]

@abayer: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

• /test pull-dogfooding-mario-test
• /test pull-tekton-plumbing-check-testgrid-config

In response to this:

/test plumbing-yamlllint

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.

[abayer]

/test plumbing-yamllint

[tekton-robot]

@abayer: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

• /test pull-dogfooding-mario-test
• /test pull-tekton-plumbing-check-testgrid-config

In response to this:

/test plumbing-yamllint

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.

[abayer]

Actually, I'd change this line to

if [ -d "$DIR" ]; then

[abayer]

fwiw, I've done the local test @lbernick described and it needs a bit of a tweak to docker run -v <path-to-repo>:/home/prow/go/src/github.com/tektoncd/pipeline -w /home/prow/go/src/github.com/tektoncd/pipeline test-runner -- ./test/presubmit-tests.sh --unit-tests.

When I run that, it works - that's what prompted me to suggest changing to -d, since using ls results in an unnecessary ls: cannot access '/home/prow/go/src/github.com/google/licenseclassifier': No such file or directory showing up in the logs. =)

[yuzp1996]

Thanks for your review @lbernick @abayer !

Following @abayer 's suggestion, I changed the way to determine if the target path exists.

[abayer]

/approve
/lgtm

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer

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:

• OWNERS [abayer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dibyom]

/ok-to-test

[yuzp1996]

@abayer Sorry, I changed the git commit message and pushed it to the repository want to rerun one of the checks but caused the label lgtm to be removed! Can you add a lgtm label when you have time? Thanks!

[abayer]

/lgtm