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 git safe.directory support #70

Merged
merged 8 commits into from
May 31, 2023
Merged

Conversation

pat-s
Copy link
Contributor

@pat-s pat-s commented May 8, 2023

fix #63

  • Add support for git config safe.directory
  • Enabled by default for the directory which is being cloned (imo this should be what users want/expect)
  • Default is set to $CI_WORKSPACE but can be adjusted via env var PLUGIN_SAFE_DIRECTORY

Works for me with image pats22/plugin-git:2.0.13 (based on plugin-git:2.0.3)

+ git init -b main
Initialized empty Git repository in /woodpecker/src/gitea.<owner>.com/<owner>/<repo>/.git/
+ git config --global safe.directory /woodpecker/src/gitea.<owner>.com/<owner>/<repo>
+ git remote add origin https://gitea.<owner>.com/<owner>/<repo>.git

main.go Outdated Show resolved Hide resolved
@lafriks lafriks added the enhancement New feature or request label May 9, 2023
@lafriks
Copy link
Contributor

lafriks commented May 10, 2023

LGTM but don't know if this will not break anything in local/ssh runners?

plugin.go Outdated Show resolved Hide resolved
@pat-s
Copy link
Contributor Author

pat-s commented May 11, 2023

LGTM but don't know if this will not break anything in local/ssh runners?

What could potentially break?

Btw, I noticed I am not the only one running into this issue though some might handle it silently: https://codeberg.org/forgejo/forgejo/src/branch/forgejo/.woodpecker/testing-amd64.yml#L53

@pat-s pat-s requested review from 6543 and lafriks May 15, 2023 21:15
@lafriks
Copy link
Contributor

lafriks commented May 15, 2023

LGTM but don't know if this will not break anything in local/ssh runners?

What could potentially break?

Btw, I noticed I am not the only one running into this issue though some might handle it silently: https://codeberg.org/forgejo/forgejo/src/branch/forgejo/.woodpecker/testing-amd64.yml#L53

When multiple jobs run on the same host in parallel one will override others safedir.

@lafriks
Copy link
Contributor

lafriks commented May 15, 2023

Maybe this could be changed to set the default safe directory only if it's not already set?

@6543
Copy link
Member

6543 commented May 16, 2023

@lafriks if it's not global set there is no race condition or change in the host config

plugin.go Outdated Show resolved Hide resolved
@lafriks
Copy link
Contributor

lafriks commented May 16, 2023

@lafriks if it's not global set there is no race condition or change in the host config

Yes, I did not notice that global is gone already ;)

@pat-s
Copy link
Contributor Author

pat-s commented May 18, 2023

Is there anything left or is this ready to merge? :)

@pat-s
Copy link
Contributor Author

pat-s commented May 22, 2023

@lafriks @6543 gentle ping :)

lafriks
lafriks previously approved these changes May 22, 2023
@lafriks lafriks requested a review from 6543 May 22, 2023 09:12
@lafriks lafriks dismissed their stale review May 29, 2023 13:51

changes in code

@6543 6543 merged commit 31b0c3d into woodpecker-ci:master May 31, 2023
@lafriks
Copy link
Contributor

lafriks commented May 31, 2023

@6543 this will now conflict for parallel jobs on local/ssh instances

@6543
Copy link
Member

6543 commented May 31, 2023

it should not ... as git global config change should not create race conditions ...

@lafriks
Copy link
Contributor

lafriks commented May 31, 2023

why not? as commands are run sequentially and if two pipelines are run at the same time it could be:

git config --global safe.directory /path/to/workspace1
git config --global safe.directory /path/to/workspace2
git clone workspace1
git clone workspace2

so there is a chance that only workspace2 pipeline will succeed

@anbraten
Copy link
Member

Afaik safe.directory is an array, so maybe it will be added to it. Otherwise we could change it to use --add

@6543
Copy link
Member

6543 commented May 31, 2023

I'm for --add if it exist to be explicite

@lafriks
Copy link
Contributor

lafriks commented May 31, 2023

As this is needed only for local/ssh I think it should be preconfigured to have safedir where all workspace directories will be created. It would be enough to check if safedir is set as parent dir for current workspace than not set it

@woodpecker-bot woodpecker-bot mentioned this pull request Jul 12, 2023
@woodpecker-bot
Copy link
Contributor

🎉 This PR is included in version 2.1.0 🎉

The release is now available [here]([object Object])

Thank you for your contribution. ❤️📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clone: "detected dubious ownership in repository"
5 participants