search

snok / install-poetry

Github action for installing and configuring Poetry
MIT License
587 stars 53 forks source link

Move main script to main.sh #70

Closed connortann closed 2 years ago

connortann commented 2 years ago

Moving main action to a dedicated shell script, to enable the ShellCheck linter to function on CI.

I have found a way to call a shell script from within the action; the slight complication is that input parameters need to be mapped as environment variables.

The script is unchanged other than replacing ${{ inputs.foo-bar }} with ${FOO_BAR} in relevant places.

I would appreciate a code review of how bash variables are used (i.e. to ensure correct use of quotes)

connortann commented 2 years ago

Any assistance fixing the failing test would be greatly appreciated! I will have some more time to look at this next week hopefully.

It appears to be due to path expansion. Might it be a "False Positive" failure? EDIT: fixed now

# test-non-default-settings
assertion failed: /home/runner/.cache/virtualenvs not found in ~/.cache/virtualenvs
sondrelg commented 2 years ago

Yes looks like adjusting the test makes most sense. I'll try my best to find time to review this tomorrow or Saturday. Looks promising 👍 Thanks for the effort @connortann!

sonarcloud[bot] commented 2 years ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

connortann commented 2 years ago

This should be ready for your review now. Apologies for all the force-pushes, I've been moving commits between different branches to test things on CI.

The test suite now passes without modification: I've ensured tildes ~ in virtualenvs.path are still expanded, as I noticed that was previously an issue #54 .

sondrelg commented 2 years ago

Looking at it again, I think the env vars might be a good idea, but probably not for this PR. I'll merge this as is. Thanks @connortann 👏

connortann commented 2 years ago

Thanks for your review @sondrelg !

I love your idea of using poetry environment variables, that would be a great simplification if it works. Could be a good future refactor.

Regarding GITHUB_ACTION_PATH, no I didn't see a confirmation in the docs that it points to the correct commit; however, I found it mentioned here as a recommended way to access files in the action repo, and it's also used in the tutorial. So, this seems like a reasonably safe thing to do: as you say I don't think anything else would make sense.