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 set-safe-directory input to allow customer to take control. #770

Merged
merged 3 commits into from Apr 21, 2022

Conversation

TingluoHuang
Copy link
Member

@TingluoHuang TingluoHuang commented Apr 19, 2022

In case of customers don't like the way we overwrite their global gitconfig with safe.directory.
Ex: customers have properly configured safe.directory on their environment, and actions/checkout overlay the wrong value.

dist/index.js Outdated Show resolved Hide resolved
thboop
thboop approved these changes Apr 19, 2022
Copy link
Collaborator

@thboop thboop left a comment

LGTM

me-and added a commit to me-and/Cygwin-Git that referenced this issue Apr 20, 2022
This fix should allow setting safe.directory manually and not having it
overridden by the actions/checkout action.  Test the behaviour works as
expected for both the Cygwin runner (with a manually set safe.directory
in the Git config file) and the Ubuntu runner (with no explicit
safe.directory wrangling).

Disable build steps that are unnecessary / slow / would produce
permanent changes.  Enable steps that we want to check with this action
even though they'd normally only be run for actual releases (subject to
the previous caveat).
@me-and
Copy link

@me-and me-and commented Apr 20, 2022

I've just kicked off a build at https://github.com/me-and/Cygwin-Git/actions/runs/2194248694, where I was hitting #767; the fix looks good, but the test should properly confirm that the fix resolves the problem I was seeing.

@me-and
Copy link

@me-and me-and commented Apr 20, 2022

No, this doesn't work, at least as a fix for #767:

Run actions/checkout@users/tihuang/setsafedirinput
  with:
    set-safe-directory: false
    repository: me-and/Cygwin-Git
    token: ***
    ssh-strict: true
    persist-credentials: true
    clean: true
    fetch-depth: 1
    lfs: false
    submodules: false
  env:
    PATH: C:\cygwin\bin
Syncing repository: me-and/Cygwin-Git
Getting Git version info
  Working directory is 'D:\a\Cygwin-Git\Cygwin-Git'
  C:\cygwin\bin\git.exe version
  git version [2](https://github.com/me-and/Cygwin-Git/runs/6090840565?check_suite_focus=true#step:4:2).[3](https://github.com/me-and/Cygwin-Git/runs/6090840565?check_suite_focus=true#step:4:3)6.0
Temporarily overriding HOME='D:\a\_temp\22b865b8-[4](https://github.com/me-and/Cygwin-Git/runs/6090840565?check_suite_focus=true#step:4:4)a74-4ad8-8903-3b[5](https://github.com/me-and/Cygwin-Git/runs/6090840565?check_suite_focus=true#step:4:5)ceaecd8[7](https://github.com/me-and/Cygwin-Git/runs/6090840565?check_suite_focus=true#step:4:7)f' before making global git config changes
Adding working directory to the temporary git global config as a safe directory
Deleting the contents of 'D:\a\Cygwin-Git\Cygwin-Git'
Initializing the repository
  C:\cygwin\bin\git.exe init D:\a\Cygwin-Git\Cygwin-Git
  hint: Using 'master' as the name for the initial branch. This default branch name
  hint: is subject to change. To configure the initial branch name to use in all
  hint: of your new repositories, which will suppress this warning, call:
  hint: 
  hint: 	git config --global init.defaultBranch <name>
  hint: 
  hint: Names commonly chosen instead of 'master' are 'main', 'trunk' and
  hint: 'development'. The just-created branch can be renamed via this command:
  hint: 
  hint: 	git branch -m <name>
  Initialized empty Git repository in /cygdrive/d/a/Cygwin-Git/Cygwin-Git/.git/
  C:\cygwin\bin\git.exe remote add origin https://github.com/me-and/Cygwin-Git
  Error: fatal: unsafe repository ('/cygdrive/d/a/Cygwin-Git/Cygwin-Git' is owned by someone else)
  To add an exception for this directory, call:
  
  	git config --global --add safe.directory /cygdrive/d/a/Cygwin-Git/Cygwin-Git
  Error: The process 'C:\cygwin\bin\git.exe' failed with exit code 12[8](https://github.com/me-and/Cygwin-Git/runs/6090840565?check_suite_focus=true#step:4:8)

It looks like it's still copying the .gitconfig file from C:\Users\runneradmin\.gitconfig, which isn't the file that's managed by Cygwin's Git.

I don't think this change is a bad change, it just doesn't resolve my problem. I think resolving #767 needs to either entirely stop actions/checkout creating a temporary "global" .gitconfig file, or it needs to get the current config by running git commands rather than assuming the location of the config file.


test-git-container:
runs-on: ubuntu-latest
container: bitnami/git:latest
Copy link
Member Author

@TingluoHuang TingluoHuang Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will have e2e coverage on any newer git version release.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

มันน่าจะดีมากขึ้นไปอีก ผมเคารพการตัดสินใจของชุมชน

@TingluoHuang TingluoHuang force-pushed the users/tihuang/setsafedirinput branch from 2839779 to 49508e7 Compare Apr 20, 2022
git config --global --add safe.directory "*"
git fetch --no-tags --depth=1 origin +refs/heads/main:refs/remotes/origin/main
- name: Fix Checkout v3
Copy link
Collaborator

@thboop thboop Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we running this again?

Copy link
Member Author

@TingluoHuang TingluoHuang Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the second checkout will wipe the initial checkout and cause post-cleanup to fail...

Copy link
Member Author

@TingluoHuang TingluoHuang Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe i can do clean:false, let me try...

Copy link
Member Author

@TingluoHuang TingluoHuang Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't work... 😢

Copy link
Collaborator

@thboop thboop Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets add a quick comment if we can just to say # needed to make post run succeed

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

จัดการได้ตามความเหมาะสม เห็นชอบ ของชุมชน

@TingluoHuang TingluoHuang force-pushed the users/tihuang/setsafedirinput branch from 718cc50 to 49508e7 Compare Apr 20, 2022
thboop
thboop approved these changes Apr 20, 2022
Copy link
Collaborator

@thboop thboop left a comment

LGTM

@TingluoHuang
Copy link
Member Author

@TingluoHuang TingluoHuang commented Apr 20, 2022

@me-and I updated the branch to make sure when set-safe-directory: false we don't create the temp global gitconfig.
Do you mind give another try to test it out? 🙇

@me-and
Copy link

@me-and me-and commented Apr 20, 2022

Yes, that seems to work! The full run hasn't completed yet, but it has got through the checkout stage using Cygwin Git.

Thank you!

@TingluoHuang TingluoHuang merged commit 0ffe6f9 into main Apr 21, 2022
11 checks passed
@TingluoHuang TingluoHuang deleted the users/tihuang/setsafedirinput branch Apr 21, 2022
TingluoHuang added a commit that referenced this issue Apr 21, 2022
* Add set-safe-directory input to allow customers to take control.
TingluoHuang added a commit that referenced this issue Apr 21, 2022
… (#776)

* Add set-safe-directory input to allow customers to take control.
jackton1 added a commit to jackton1/github-push-action that referenced this issue Jun 14, 2022
This has been resolved in the [checkout](actions/checkout#770) action and would already be set by default.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants