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

GitHub Action: avoid extra working directory prefix for wget #4232

Closed
wants to merge 1 commit into from

Conversation

ptoscano
Copy link
Contributor

The whole shell step for wget is run with the working directory for the action; hence, drop the extra prefix to the --output-document parameter of the wget command.

The whole shell step for wget is run with the working directory for the
action; hence, drop the extra prefix to the --output-document parameter
of the wget command.
@ptoscano ptoscano requested a review from a team as a code owner June 26, 2024 14:24
@ptoscano ptoscano requested review from ssbarnea and alisonlhart and removed request for a team June 26, 2024 14:24
@richm
Copy link

richm commented Jun 26, 2024

we fixed this in system roles by using an absolute path for the working directory (for ansible-lint and ansible-test also) - linux-system-roles/kernel_settings#206

Copy link
Contributor

@bkaraoren bkaraoren left a comment

Choose a reason for hiding this comment

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

When you run this on a container it was not getting the correct working-directory from GITHUB environment variables. GITHUB environment variables are only available for the RUNNER, not on a container in RUNNER. Please check the previous requirements by the changes. Please check the previous commits where there was no path and it was also not working correctly.

@ptoscano
Copy link
Contributor Author

@bkaraoren I think you misunderstand what this PR does: the shell run block used to invoke wget already has the working_directory parameter set, so using it again in the output filename of wget is not correct.

What you describe sounds like a problem in the detection of the default working directory in case the working_directory parameter of this action is not specified (see the first set of the action, currently labelled "Process inputs"). In that case, I think that automatic detection ought to be fixed.

@bkaraoren
Copy link
Contributor

tection ought to be fixe

@ptoscano yep, that sounds also good. without providing any directory such as ${{ steps.inputs.outputs.working_directory }} I also expect it to work but former commit needs to be checked why it has been reverted back from not using any working directory in the commit #3938. Could be useful to check it to understand the reason what was the reason to include.

Copy link

sonarcloud bot commented Jul 24, 2024

Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

Based on given reviews, I think that is better not to touch it unless the change is also adding tests, especially for called via container approach. Without this, we will probably close the PRs.

@tigattack
Copy link

We're going in circles here; this PR brings us back to the same state as before #4103.

For myself and others using a GitHub action in a repository subdirectory, #4103 addressed a major issue (see #3938, now #4323). However, it also introduced problems for other users.

In response, #4213 was merged, but this caused the action to become unusable again without resorting to hacky workarounds for the same use case.

Instead of continuing in this cycle, we should take a step back and reevaluate the approach, as each "fix" solves one group's problem but introduces a new breaking bug for another.

@ssbarnea
Copy link
Member

@tigattack @bkaraoren @ptoscano Please check #4340 which will fix this issue while also adding regression tests. Once merged this PR will be closed. I will release it later today, hopefully addressing these long standing issues.

@tigattack
Copy link

tigattack commented Sep 20, 2024

This change looks great and makes me much more confident, however it appears to have caused an issue with the Python deps install for me - See here.
Not intending to "bump", as I did already comment on the PR, just felt FYI-worthy for anyone tracking the issue.

Edit: Solved by #4342.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants