Cleanup of initializing working dirs #843

abayer · 2019-05-08T12:48:19Z

Changes

Subsumes #726, to fix #725, with a unit test fix and a tweak to make sure that
artifact_bucket_test doesn't try to run in parallel, since that messes
up various PipelineRun tests.

Note that I can't reproduce the e2e test failure seen in #726 locally, at least. We'll see if it pops up here.

Submitter Checklist

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

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

Release Notes

n/a

In order to allow steps to define workingDirs that are subdirectories of the workspace directory we need to make sure they have been created first since they will not exist on startup. Fixes tektoncd#725

…icarlo2-workdir

Subsumes tektoncd#726, with a unit test fix and a tweak to make sure that artifact_bucket_test doesn't try to run in parallel, since that messes up various PipelineRun tests. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

googlebot · 2019-05-08T12:48:21Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

abayer · 2019-05-08T12:49:22Z

The CLA bot is definitely not liking multi-author PRs.

googlebot · 2019-05-08T12:49:53Z

☹️ Sorry, but only Googlers may change the label cla: yes.

abayer · 2019-05-08T12:50:10Z

awwwww I can't even set the CLA flag.

abayer · 2019-05-08T12:52:12Z

cc @dicarlo2 since I lifted your PR. =)

/assign @dlorenc
/assign @bobcatfish
/assign @imjasonh
...for help with the CLA label if nothing else. =)

abayer · 2019-05-08T13:01:34Z

Well, nice, looks like the override-with-bash-noop:latest image is working here.

abayer · 2019-05-08T13:22:59Z

dangit, it's back, but only in the yaml tests. I am confused.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

abayer · 2019-05-08T13:46:26Z

Ok, this should actually be fixed now - tested locally.

dlorenc · 2019-05-08T13:49:23Z

I manually verified the CLAs are correct. Overriding the bot.

googlebot · 2019-05-08T13:49:35Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-08T14:26:38Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

abayer · 2019-05-08T14:26:48Z

Thanks, @dlorenc!

abayer · 2019-05-08T15:00:47Z

The CLA bot is still confused, but eh, that can wait.

googlebot · 2019-05-08T15:35:18Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

dlorenc · 2019-05-08T15:37:05Z

/lgtm

dlorenc · 2019-05-08T15:37:29Z

/approve

tekton-robot · 2019-05-08T15:37:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, dlorenc

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~~ [dlorenc]

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

vdemeester · 2019-05-08T16:25:14Z

/lgtm

bobcatfish · 2019-05-08T19:33:17Z

test/artifact_bucket_test.go

@@ -48,7 +48,7 @@ func TestStorageBucketPipelineRun(t *testing.T) {
 		t.Skip("GCP_SERVICE_ACCOUNT_KEY_PATH variable is not set.")
 	}
 	c, namespace := setup(t)
-	t.Parallel()
+	// Bucket tests can't run in parallel without causing issues with other tests.


what kind of issues?

The TestPipelineRun tests hang for me when run in parallel with TestStorageBucketPipelineRun - my guess is that when they get created, the ConfigMap is set up to use a bucket, but by the time they actually get going, the bucket has been deleted due to this test finishing.

dicarlo2 and others added 3 commits May 3, 2019 16:09

Initialize working dirs inside workspace dir

2f3691e

In order to allow steps to define workingDirs that are subdirectories of the workspace directory we need to make sure they have been created first since they will not exist on startup. Fixes tektoncd#725

Merge branch 'workdir' of https://github.com/dicarlo2/pipeline into d…

791729c

…icarlo2-workdir

Cleanup of initializing working dirs

8744ae4

Subsumes tektoncd#726, with a unit test fix and a tweak to make sure that artifact_bucket_test doesn't try to run in parallel, since that messes up various PipelineRun tests. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

tekton-robot requested review from dlorenc and imjasonh May 8, 2019 12:48

googlebot added the cla: no label May 8, 2019

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 8, 2019

abayer added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels May 8, 2019

googlebot removed the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label May 8, 2019

abayer added the cla: no label May 8, 2019

tekton-robot assigned bobcatfish, dlorenc and imjasonh May 8, 2019

abayer added 2 commits May 8, 2019 09:36

Flag for bash noop image should be the one used in controller

37c0918

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

Duplicate declarations of flags makes things break

c2bc19d

dlorenc added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label May 8, 2019

googlebot removed the cla: no label May 8, 2019

Missed a reference to bashWorkingDirImage, switched it to bashNoopImage

06c9a3c

googlebot added cla: no and removed cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit labels May 8, 2019

dlorenc added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels May 8, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 8, 2019

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2019

vdemeester removed lgtm Indicates that a PR is ready to be merged. labels May 8, 2019

tekton-robot assigned vdemeester May 8, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 8, 2019

tekton-robot merged commit 8999271 into tektoncd:master May 8, 2019

vdemeester mentioned this pull request May 8, 2019

Initialize working dirs inside workspace dir #726

Closed

3 tasks

bobcatfish reviewed May 8, 2019

View reviewed changes

abayer mentioned this pull request May 10, 2019

When adding a chart to an environment build-step-build-helm-build fails jenkins-x/jx#3854

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup of initializing working dirs #843

Cleanup of initializing working dirs #843

abayer commented May 8, 2019 •

edited

Loading

googlebot commented May 8, 2019

abayer commented May 8, 2019

googlebot commented May 8, 2019

abayer commented May 8, 2019

abayer commented May 8, 2019

abayer commented May 8, 2019

abayer commented May 8, 2019

abayer commented May 8, 2019

dlorenc commented May 8, 2019

googlebot commented May 8, 2019

googlebot commented May 8, 2019

abayer commented May 8, 2019

abayer commented May 8, 2019

googlebot commented May 8, 2019

dlorenc commented May 8, 2019

dlorenc commented May 8, 2019

tekton-robot commented May 8, 2019

vdemeester commented May 8, 2019

bobcatfish May 8, 2019

abayer May 8, 2019

Cleanup of initializing working dirs #843

Cleanup of initializing working dirs #843

Conversation

abayer commented May 8, 2019 • edited Loading

Changes

Submitter Checklist

Release Notes

googlebot commented May 8, 2019

abayer commented May 8, 2019

googlebot commented May 8, 2019

abayer commented May 8, 2019

abayer commented May 8, 2019

abayer commented May 8, 2019

abayer commented May 8, 2019

abayer commented May 8, 2019

dlorenc commented May 8, 2019

googlebot commented May 8, 2019

googlebot commented May 8, 2019

abayer commented May 8, 2019

abayer commented May 8, 2019

googlebot commented May 8, 2019

dlorenc commented May 8, 2019

dlorenc commented May 8, 2019

tekton-robot commented May 8, 2019

vdemeester commented May 8, 2019

bobcatfish May 8, 2019

Choose a reason for hiding this comment

abayer May 8, 2019

Choose a reason for hiding this comment

abayer commented May 8, 2019 •

edited

Loading