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

[FR]: Providing working-directory as an input param #98

Merged

Conversation

yudintsevegor
Copy link
Contributor

@yudintsevegor yudintsevegor commented Jan 5, 2025

Is your feature request related to a problem? Please describe.
We would like to use this github action for the directory with subdirectories, which contain go.* files.
Unfortunately, the action doesn't work if the root directory doesn't contain go.* files. It forces as having steps in github actions - run: mv ./* ./../../

Describe the solution you'd like
I guess, the solution might be an optional working-directory input param with a default value ${{ github.workspace }}.

Describe alternatives you've considered
Honestly, i haven't thought of any better solution for the users, but the implementation might be improved - I am not so experienced with shell scripts.

Additional context
The error which we got:

6:56PM ERR Scanning yielded error error="running govulncheck binary produced govulncheck: no go.mod file\n\ngovulncheck only works with Go modules. Try navigating to your module directory.\nOtherwise, run go mod init to make your project a module.\n\nSee https://go.dev/doc/modules/managing-dependencies for more information.\n"

image

Implementation notes:

  • the input param working-directory with default value ${{ github.workspace }} was provided
  • the script determine-wd.sh was added to set correctly working directory for the github action and for the docker container as they are a little bit different (see below)
  • --workdir and -v can't use relative path - it should be provided as absolute ones (e.g. /kek/lol instead of ./kek/lol)
  • run.working-directory can use only relative paths, that's why we can't use the same value for docker ... -v and docker ... --workdir and run.working-directory (https://docs.github.com/en/actions/sharing-automations/creating-actions/metadata-syntax-for-github-actions#runsstepsworking-directory)

e2e testing:
yudintsevegor/playground#1
f.e.: yudintsevegor/playground@f6a3b51

Concerns:

  • the current implementation allows the user to provide both absolute and relative paths: working-directory: ./kek/lol and working-directory: /kek/lol are both valid. I wonder if it's fine 🤔

Notes:

  • if it's approved by the owner of the repo, I am happy to try to provide some tests for this impl. I tested it in our private repo for now.

@yudintsevegor yudintsevegor changed the title feat: provide working-directory as an input param [FR]: Providing working-directory as an input param Jan 5, 2025
@yudintsevegor yudintsevegor marked this pull request as ready for review January 5, 2025 21:02
@yudintsevegor
Copy link
Contributor Author

Hey @Templum!

I can't modify Labels and Assignees to assign the correct things there 🤔

@Templum Templum self-assigned this Jan 5, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.20%. Comparing base (e7476a5) to head (f7b0e70).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #98   +/-   ##
=======================================
  Coverage   70.20%   70.20%           
=======================================
  Files           5        5           
  Lines         433      433           
=======================================
  Hits          304      304           
  Misses        124      124           
  Partials        5        5           
Flag Coverage Δ
unit-tests 70.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Templum
Copy link
Owner

Templum commented Jan 5, 2025

Can you perform an E2E Run against your change and link the result here ? Sadly the testing I have does not cover E2E (only once merged). And given this change does not touch "code" the existing tests aren't helpful

Comment on lines +6 to +18
if [ -z $github_workspace ]; then
echo "The first argument (github.workspace) is required.";
exit 1;
elif [[ $github_workspace = $input_working_directory || -z $input_working_directory ]]; then
export DOCKER_WD=$github_workspace;
export GITHUBH_WD=$github_workspace;
elif [[ $input_working_directory =~ ^/ ]]; then
export DOCKER_WD=$input_working_directory;
export GITHUBH_WD=.$input_working_directory;
else
export DOCKER_WD=$(echo $input_working_directory | sed 's/\.//');
export GITHUBH_WD=$input_working_directory
fi
Copy link
Owner

Choose a reason for hiding this comment

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

Would you be so kind to walk me through your code here ?

I see that DOCKER_WD & GITHUBH_WD in the most cases have the same content

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure!

The content is not exactly the same. Let me provide more details, which could simply the understanding:

  • if the provided github.worspace is empty --> the execution is not possible. The reason for that is to be sure that the first argument is github.worspace as we use it as a default case.
  • if the github_workspace == input_working_directory OR the input_working_directory == "" (as a second arg) we could use github_workspace as a default use case.

if 2 args are provided, we could forget about github_workspace and use input_working_directory for our processing:

  • if input_working_directory starts with / then for the docker we would use it as it is (docker accepts only absolute paths), but for the github workdir we would need to add a dot as a prefix: ./ (GitHub working-directory should be relative path)
  • input_working_directory starts with ./ then for the docker we would remove a dot from the beginning with a help of sed command (docker accepts only absolute paths) and use input_working_directory as it is for the github actions (GitHub working-directory should be relative path)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This morning I tried to implement it in Go with tests (you could see commits below), but I gave up this idea because it's not possible to set the env variable from the go program to use it outside of it. I guess it could be a workaround, but probably with sh script is cleaner

Copy link
Owner

Choose a reason for hiding this comment

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

No worries, probably having a shell script is fine for this case. I also don't think any extra test needs to be done. The E2E Flow that we have in place, should be sufficient enough.

@yudintsevegor
Copy link
Contributor Author

yudintsevegor commented Jan 5, 2025

@Templum wow, thank you so much for the quick response! I will back to the PR a little bit later 😅
I didn't expect you to answer so fast

@yudintsevegor yudintsevegor marked this pull request as draft January 6, 2025 11:46
@yudintsevegor
Copy link
Contributor Author

@Templum

Can you perform an E2E Run against your change and link the result here ? Sadly the testing I have does not cover E2E (only once merged). And given this change does not touch "code" the existing tests aren't helpful

Do you mean create a repo to use this workflow or something else?

@yudintsevegor yudintsevegor requested a review from Templum January 6, 2025 12:17
@yudintsevegor yudintsevegor marked this pull request as ready for review January 6, 2025 12:17
@Templum
Copy link
Owner

Templum commented Jan 6, 2025

@Templum

Can you perform an E2E Run against your change and link the result here ? Sadly the testing I have does not cover E2E (only once merged). And given this change does not touch "code" the existing tests aren't helpful

Do you mean create a repo to use this workflow or something else?

Exactly, you could use your repo that was used to produce the screenshot. And specify this branch and let it run

@yudintsevegor yudintsevegor marked this pull request as draft January 6, 2025 15:19
@yudintsevegor
Copy link
Contributor Author

@Templum yudintsevegor/playground#1 check this :)

the commit with a branch: yudintsevegor/playground@f6a3b51

@yudintsevegor yudintsevegor marked this pull request as ready for review January 6, 2025 16:51
Copy link
Owner

@Templum Templum left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, it looks good to me. The linked test runs where also successful and did not show any issues. From what I can tell, this change should not be breaking any previous behaviour

@Templum
Copy link
Owner

Templum commented Jan 6, 2025

@yudintsevegor thanks for your contribution, I will merge it later. And then eventually release it. However, last release had some issue... Which I will need to read up on.

@Templum Templum merged commit 0f569df into Templum:main Jan 6, 2025
4 checks passed
@yudintsevegor
Copy link
Contributor Author

@Templum thank you so much for the quick cooperation!

I haven't updated the docs and didn't update the version. Shall I do it or you want to do it on your own? 👀

@Templum
Copy link
Owner

Templum commented Jan 6, 2025

@Templum thank you so much for the quick cooperation!

I haven't updated the docs and didn't update the version. Shall I do it or you want to do it on your own? 👀

You can follow the release progress over @ #100, will need to address the issue of the previous release attempt. The documentation I already adjusted based on what you did in the action.yaml.

@Templum
Copy link
Owner

Templum commented Jan 7, 2025

This was released as part of v1.0.2

yudintsevegor added a commit to yudintsevegor/playground that referenced this pull request Jan 8, 2025
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