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

[BUGFIX] Provide gitPath for Windows to avoid failures on windows-2022 (GitHub-hosted runner) #137

Merged
merged 4 commits into from
Oct 19, 2022

Conversation

ochococo
Copy link
Contributor

Problem:

Observed error on windows-2022 (GitHub-hosted runner) that git command cannot be found.

Issue:

Cannot find git executable on on windows-2022 (GitHub-hosted runner) #136

Solution:

This path improvement makes use of existing path.js to resolve and return correct git.exe path for Windows, leaving the executable name as it was for other operating systems.

Caveats:

No idea how and why this c://progra~1//git//usr//bin//git.exe mumbo-jumbo works but it apparently did for other executables so figured it should work for git.exe (and it does).

@ochococo
Copy link
Contributor Author

@sebastiankugler 👀 please 🙂

Copy link
Member

@sebastiankugler sebastiankugler left a comment

Choose a reason for hiding this comment

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

Seems reasonable, correct and in line with how we treat the other executables.

index.js Outdated Show resolved Hide resolved
@ochococo
Copy link
Contributor Author

All checks have failed, should I worry @sebastiankugler ?

@sebastiankugler
Copy link
Member

We need to understand ourselves, but we're on it! No need to worry 🙂

@mpdude
Copy link
Member

mpdude commented Oct 19, 2022

I'm a bit concerned regarding BC: What about people using self-hosted Windows runners where git is in the PATH, but not at the absolute path given here?

Should we, in general, make all the paths configurable through action inputs?

Would a CHANGELOG entry be good enough, given that we're still at 0.x releases?

Could paths.js more specifically detect GitHub-managed windows-2022 runners and apply these paths only then?

@ochococo
Copy link
Contributor Author

I'm a bit concerned regarding BC: What about people using self-hosted Windows runners where git is in the PATH, but not at the absolute path given here?

Same here but if current strategy is wrong, this PR won't make it any worse, right? Why would we use agent related from current absolute path and git exec from another location?

Should we, in general, make all the paths configurable through action inputs?

That would be definitely a major improvement, one can only benefit from such flexibility. Seems like a bigger feature and effort.

@mpdude
Copy link
Member

mpdude commented Oct 19, 2022

Same here but if current strategy is wrong, this PR won't make it any worse, right? Why would we use agent related from current absolute path and git exec from another location?

Yeah, valid point. Every change breaks someone's workflow, but maybe we can accept this risk here.

dist/cleanup.js Outdated
Comment on lines 2837 to 2840
homePath: os.homedir(),
sshAgentPath: 'c://progra~1//git//usr//bin//ssh-agent.exe',
sshAddPath: 'c://progra~1//git//usr//bin//ssh-add.exe',
gitPath: 'c://progra~1//git//usr//bin//git.exe'
Copy link
Member

@mpdude mpdude Oct 19, 2022

Choose a reason for hiding this comment

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

@ochococo Now that I take a step back, what would you think about having homePath, but sshAgentCmd (or ...Command) and the same for other items that refer to binaries?

I can make the change for you if you agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would then argue this is not a command but rather a path to command (or executable if you will).
Bike-shedding obviously, not a strong objection from me, UTY

@mpdude
Copy link
Member

mpdude commented Oct 19, 2022

The problem with the workflows probably is that secrets are not available when the workflows are started for PRs from external contributors. Not sure yet how to best address this.

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.

3 participants