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

Allow python-version-file to be a relative path #431

Merged
merged 2 commits into from Jun 20, 2022

Conversation

Kurt-von-Laven
Copy link
Contributor

@Kurt-von-Laven Kurt-von-Laven commented Jun 12, 2022

Description:
Don't assume that it is safe to prepend the GITHUB_WORKSPACE environment variable to the given path since the path may already be absolute.

Related issue:
Fixes #428.
Fixes #430.

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@vsafonkin
Copy link
Contributor

vsafonkin commented Jun 13, 2022

@Kurt-von-Laven, probably it will be better to add a check for python-version-file input is absolute or relative:
https://nodejs.org/api/path.html#pathisabsolutepath

And skip joining with GITHUB_WORKSPACE var if a path from this input is absolute. What do you think about it?

@Kurt-von-Laven
Copy link
Contributor Author

Kurt-von-Laven commented Jun 13, 2022

I can’t think of a benefit the extra branch would confer, but I certainly don’t claim comprehensive knowledge of GitHub Actions. My understanding is that GITHUB_WORKSPACE is already the default working directory of steps. It’s nice to be able to override this default by setting the working-directory explicitly. Notably, in composite actions GITHUB_WORKSPACE is usually the wrong default working directory in my experience.

@dmitry-shibanov
Copy link
Contributor

dmitry-shibanov commented Jun 15, 2022

Hello @Kurt-von-Laven. I've tried to reproduce the behaviour with composite actions but it works as expected. Is uses the same GITHUB_WORKSPACE as the main job. Could you please provide a public repository to reproduce it ?

@Kurt-von-Laven
Copy link
Contributor Author

Kurt-von-Laven commented Jun 15, 2022

We are saying the same thing. The default working directory when not specified by explicitly setting working-directory is always GITHUB_WORKSPACE. For this reason alone, it seems odd to explicitly prepend GITHUB_WORKSPACE even if only to relative paths, because as far as I can see doing so never adds any value. The bigger problem though is that the action effectively hardcodes the working directory, causing the user's explicitly specified choice of working-directory to be silently ignored. This is most problematic in my experience within composite actions, where the default working directory of GITHUB_WORKSPACE frequently needs to be overridden to instead be the directory the composite action was checked out to.

@dmitry-shibanov
Copy link
Contributor

dmitry-shibanov commented Jun 16, 2022

Hello @Kurt-von-Laven. Could you please sync with the main branch ?

Kurt-von-Laven and others added 2 commits Jun 16, 2022
Don't assume that it is safe to prepend the GITHUB_WORKSPACE
environment variable to the given path since the path may already be
absolute.
@Kurt-von-Laven
Copy link
Contributor Author

Kurt-von-Laven commented Jun 16, 2022

Thank you for the review everyone! I rebased on main.

@dmitry-shibanov dmitry-shibanov merged commit ffcd000 into actions:main Jun 20, 2022
114 of 119 checks passed
@Kurt-von-Laven Kurt-von-Laven deleted the patch-2 branch Jun 21, 2022
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.

Can't Specify python-version-file as Absolute Path Log statement omits default Python version file
5 participants