woodpecker-ci / woodpecker

Woodpecker is a simple, yet powerful CI/CD engine with great extensibility.
https://woodpecker-ci.org
Apache License 2.0
4.3k stars 371 forks source link

Respect `directory` option for steps again #4319

Closed 6543 closed 2 weeks ago

6543 commented 2 weeks ago

close #4314

6543 commented 2 weeks ago

-> not tested jet but should fix it

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 71.42857% with 8 lines in your changes missing coverage. Please review.

Project coverage is 26.55%. Comparing base (b0f52b9) to head (0e3e42b). Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
pipeline/backend/common/script_posix.go 42.85% 3 Missing and 1 partial :warning:
pipeline/backend/common/script_win.go 55.55% 3 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #4319 +/- ## ========================================== + Coverage 26.52% 26.55% +0.03% ========================================== Files 377 379 +2 Lines 27448 27548 +100 ========================================== + Hits 7281 7316 +35 - Misses 19503 19563 +60 - Partials 664 669 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lafriks commented 2 weeks ago

I did not test but I don't see why working directory be anyhow related to workspace or permissions (it could be of course relative but not limited to)

pat-s commented 2 weeks ago

As said in my previous comment, it is about the mount point and its permissions which is different for subdirs on k8s. I wouldn't have changed it (i.e. get rid of workDir) otherwise in the previous/initial PR.

While I am not saying that this PR won't work per se (before I tested it), I am not so happy about an (early) approval here without any (practical) tests (given the impact and size of the previous PR) without waiting for at least an reply from the former PR creator (which in this case happens to be me but that doesn't matter overall). I.e, if I wouldn't have put a "please wait" just shortly after, this one would have likely already been merged.

pat-s commented 2 weeks ago

On a related note: as this is all just because of directory:: arguably this is a niche param which almost not used anywhere and not doing much else than cd to a subdir (for often no reason). If this happens to cause general issues for the whole rootless approach, I'd be in strong favor of removing it.

lafriks commented 2 weeks ago

On a related note: as this is all just because of directory:: arguably this is a niche param which almost not used anywhere and not doing much else than cd to a subdir (for often no reason). If this happens to cause general issues for the whole rootless approach, I'd be in strong favor of removing it.

I strongly disagree on this, we use it quite often, especially since for plugins you can't specify commands option to cd subdir otherwise plugin itself won't run and would not be secure anymore but there are many cases where for example reviewdog plugins where you need to run eslint only for subdir where frontend code is located and golangci-lint on directory where backend code is, it's also way more clean this way

lafriks commented 2 weeks ago

As said in my previous comment, it is about the mount point and its permissions which is different for subdirs on k8s. I wouldn't have changed it (i.e. get rid of workDir) otherwise in the previous/initial PR.

While I am not saying that this PR won't work per se (before I tested it), I am not so happy about an (early) approval here without any (practical) tests (given the impact and size of the previous PR) without waiting for at least an reply from the former PR creator (which in this case happens to be me but that doesn't matter overall). I.e, if I wouldn't have put a "please wait" just shortly after, this one would have likely already been merged.

I'm sorry for a bit rushed approval but initially code looked good code wise and as this is something that is pretty much regression for me so I approved it 😊

6543 commented 2 weeks ago

The direcotory is important for plugins yes :)

I'd like to see both working (rootles & dir) ... I tested docker backend witch works just fine with workdir and an non-root user.

For k8s I need to test things, is pull here is first and formost to discusst getting a good solution.

woodpecker-bot commented 2 weeks ago

Tearing down https://woodpecker-ci-woodpecker-pr-4319.surge.sh