Add workingdir integration test

[chmouel]

Add an integration test checking the workingdirs

Following on review #1609 (review) we realised that there wasn't any integration test for the workingdir step, let's add some tests for this.

Closes #1624

Changes

We didn't check previously if workingdir really worked properly, let's add tests
for this.

/cc @imjasonh

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)
• Commit messages follow commit message best practices

[chmouel]

I could not find a way to do a TestBad I need to figure out how to not being able to write to /workspace since it's volume mounted, fs mode rights are assigned to our running user (so having a new SA with non elevated priv would not work.

[imjasonh]

Would it be possible to phrase at least some of this test as a YAML test?

steps:
- image: ubuntu
  command: ['touch', 'foo']
  workingDir: 'foo'  # implicitly mkdir's /workspace/foo
- image: ubuntu
  command: ['cat', '/workspace/foo/foo']
...

This has the benefit of being easier (I think) to read and understand in isolation.

[chmouel]

Good idea, updating the existing workdir.yaml with something like this? :

diff --git a/examples/taskruns/workingdir.yaml b/examples/taskruns/workingdir.yaml
index e8a7d87f..73229cf8 100644
--- a/examples/taskruns/workingdir.yaml
+++ b/examples/taskruns/workingdir.yaml
@@ -12,6 +12,14 @@ spec:
       - '-c'
       - '[[ $PWD == /workspace ]]'
 
+    - name: default
+      image: ubuntu
+      command: ['bash']
+      workingDir: "foo"  # implicitly mkdir's /workspace/foo
+      args:
+      - '-c'
+      - '[[ $PWD == /workspace/foo ]]'
+
     - name: override
       image: ubuntu
       command: ['bash']

[imjasonh]

Yeah that sounds great. Is there anything else left in the Go test that can be YAMLified?

[chmouel]

@imjasonh I think the go test are still useful they make it implicit what should be expected, I

I meant we need to make sure no output is coming back out of the mkdir -p and i don't thin kthe YAML would catch this?

There is another test in there too which make sure that workingdir with absolute path (ie /foo) that are not starting with /workspace.

[imjasonh]

@imjasonh I think the go test are still useful they make it implicit what should be expected, I

I meant we need to make sure no output is coming back out of the mkdir -p and i don't thin kthe YAML would catch this?

It wouldn't. I'm just not sure how much that matters though. The test implicitly checks that the directory exists as expected, that's all I really care about.

[bobcatfish]

@chmouel could you add some more detail in the commit message about what this commit is for? at the moment it just says "Add workingdir integration test" but it's hard to understand without context. I don't know what "workingdir integration test" is, and even from #1624 I'm not really sure what the workingdir feature is :S

Sorry for the nitpicking!! But having a lot of info in our commit messages and in our issues is really important to me, and makes it a lot easier to make sure all the contributors can be on the same page.

@chmouel (or anyone) feel free to remove this hold once we have some more details:
/hold

[chmouel]

@bobcatfish I kinda rephrased it, but would you like me to explain what the workingdir step is doing in the commit message? I think that would be a documentation update, isnt it ?

[chmouel]

CI error is unrelated to this commit but this #1634 would fix it,

[bobcatfish]

Thanks for the update! 🙏

@bobcatfish I kinda rephrased it, but would you like me to explain what the workingdir step is doing in the commit message? I think that would be a documentation update, isnt it ?

I mean I would! I like to err on the side of being verbose, cuz it's really hard to know how much you'll know later - for example, say someone is looking at this commit message 2 years from now and the workingdir step is long gone but some of the lines of code remain, they'll have a really hard time understanding this change

[chmouel]

@bobcatfish I understand and if I could I would have link to the documentation from the commit message to explains what the workingdir but unfortunately it's not there :(

[chmouel]

/retest

[chmouel]

It doesnt not seem that a relative path for workingdir can actually work 🤔

35s Warning Failed pod/workingdir-2hcbb-pod-366869 Error: container create failed: Cwd must be an absolute path

[vdemeester]

It doesnt not seem that a relative path for workingdir can actually work thinking

35s Warning Failed pod/workingdir-2hcbb-pod-366869 Error: container create failed: Cwd must be an absolute path

Yeah that make sense 👼

[chmouel]

I guess this code is not doing what expected then :

https://github.com/chmouel/tektoncd-pipeline/blob/34db59dd10f6407de6427992b2d2b04c4208027b/pkg/pod/workingdir_init.go#L60-L65

It should be

	if filepath.IsAbs(p) || strings.HasPrefix(p, "/workspace/") {

(currently it skips the absolute path like /foo and catch the non absolute path like foo)

[chmouel]

/hold cancel

[chmouel]

CB> I guess this code is not doing what expected then :

I'll work on a following PR to fix the code,

[vdemeester]

/lgtm

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

• OWNERS [vdemeester]

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